hyperformula icon indicating copy to clipboard operation
hyperformula copied to clipboard

Make signatures of `simpleCellAddressToString` and `simpleCellRangeToString` more logical

Open sequba opened this issue 2 years ago • 3 comments

Description

Currently sheetId needs to be passed twice to simpleCellAddressToString:

const cellAddress = { sheet: sheetId, row: 0, col: 3};
const cellAddressAsString = hfInstance.simpleCellAddressToString(cellAddress, sheetId);

~~The second argument to simpleCellAddressToString should be optional. Do not remove it completely to keep backwards compatibility.~~ See comments

Links

sequba avatar Jan 24 '23 13:01 sequba

For a reason. Those are not the same. See #22 for a discussion on this topic.

You can find examples in tests:

expect(simpleCellAddressToString(sheetIndexMappingFunction, adr('A1'), 0)).toEqual('A1')
expect(simpleCellAddressToString(sheetIndexMappingFunction, adr('A1'), 1)).toEqual('Sheet1!A1')

IMO making it optional is possible but will be harmful. It only solidifies misunderstanding of references. But if you decide to make it optional anyway, please keep in mind that it is a functionality, should not be interpreted as backward compatibility or removed in the future releases.

wojciechczerniak avatar Jan 25 '23 18:01 wojciechczerniak

@wojciechczerniak thank you for the clarification. I think I got it.

The second argument of simpleCellAddressToString is a sheet context. The sheet name is included in the resulting string if and only if the context is different than the cellAddress.sheet.

In that case: First of all, the documentation is wrong:

@param {number} sheetId - context used in case of missing sheet in the first argument

Second, I'd rather change the signature of this function to something like this (but that would be a breaking change):

public simpleCellAddressToString(cellAddress: SimpleCellAddress, options: { includeSheetName?: boolean } = {}): string | undefined {

Third, the analogous changes should be applied to simpleCellRangeToString

sequba avatar Jan 26 '23 09:01 sequba

Let's change the method signature but keep the old version still working. The second parameter should be { includeSheetName?: boolean } | number

sequba avatar Apr 04 '24 13:04 sequba