[REM] merge: remove useless topLeft attribute from Merge interface

[Rachico]

Description:

merge: remove useless topLeft attribute from Merge interface.

Context

The "topLeft" attribute in the Merge interface was used as a shortcut to get the top left position of the merge zone. However, this was creating redundant code.

Task: 3912050

review checklist

• feature is organized in plugin, or UI components
• support of duplicate sheet (deep copy)
• in model/core: ranges are Range object, and can be adapted (adaptRanges)
• in model/UI: ranges are strings (to show the user)
• undo-able commands (uses this.history.update)
• multiuser-able commands (has inverse commands and transformations where needed)
• new/updated/removed commands are documented
• exportable in excel
• translations (_t("qmsdf %s", abc))
• unit tested
• clean commented code
• track breaking changes
• doc is rebuild (npm run doc)
• status is correct in Odoo

[robodoo]

Pull request status dashboard.

[VincentSchippefilt]

small nit picking, otherwise all good. Thanks

[VincentSchippefilt]

Suggested change

	col: mergeZone?.left as HeaderIndex,
	row: mergeZone?.top as HeaderIndex,
	col: mergeZone!.left,
	row: mergeZone!.top,

No need to re-cast into HeaderIndex, you can tell typescript it has to have a value with !

[pro-odoo]

History has shown us that each time we introduce a !, a future bug is also introduce. ! is a way to tell typescript to have truth in you. It could be correct now, but could become incorrect in the future and we will not notice it. The right way to handle this is to check that mergeZone is defined, and you did it correctly. So, simply remove the cast and optional:

col: mergeZone.left,
row: mergeZone.top,

[pro-odoo]

Note that ! can be used in tests

[hokolomopo]

As a general rule we should avoid casting unless there is no other choice.

An if(mergeZone) just above instead of the if(this.isInMerge) would fix that.

Note that this wasn't needed because we had an ! before, which is something we should also avoid

[VincentSchippefilt]

Suggested change

	const mergeZone = this.getMerge(position);
	return {
	sheetId: position.sheetId,
	col: mergeZone?.left as HeaderIndex,
	row: mergeZone?.top as HeaderIndex,
	};
	const mergeZone = this.getMerge(position);
	return mergeZone ? {
	sheetId: position.sheetId,
	col: mergeZone.left,
	row: mergeZone.top
	} : position;

[LucasLefevre]

casting is useless here (and should always be avoided when possible).

Suggested change

	sheetId: position.sheetId,
	col: mergeZone?.left as HeaderIndex,
	row: mergeZone?.top as HeaderIndex,
	sheetId: position.sheetId,
	col: mergeZone.left,
	row: mergeZone.top,

[LucasLefevre]

Check the entire merge and not only the first one

Suggested change

	const mergeZone = getMerges(model)[1];
	expect({ col: mergeZone.left, row: mergeZone.top }).toEqual(toCartesian("A2"));
	expect(getMerges(model)).toEqual([toZone("A2:B2")]);

[Rachico]

@LucasLefevre it's not a list, it's an object with the ids of merges as keys

[LucasLefevre]

Ah ok,
then you can still check the entire merge

expect(mergeZone).toEqual(toZone("A2:B2"));

[rrahir]

@robodoo r+