Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make signatures of simpleCellAddressToString and simpleCellRangeToString more logical #1151

Open
sequba opened this issue Jan 24, 2023 · 3 comments · Fixed by #1457
Open
Assignees
Labels
Feature Something we can add later on without introducing a breaking change Impact: Low

Comments

@sequba
Copy link
Contributor

sequba commented Jan 24, 2023

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 sequba added the Feature Something we can add later on without introducing a breaking change label Jan 24, 2023
@sequba sequba self-assigned this Jan 24, 2023
@wojciechczerniak
Copy link
Contributor

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.

@sequba
Copy link
Contributor Author

sequba commented Jan 26, 2023

@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 sequba changed the title Make sheetId optional in simpleCellAddressToString method Fix documentation of simpleCellAddressToString and simpleCellRangeToString Jan 26, 2023
@sequba sequba added the Docs Improvements or additions to documentation label Mar 22, 2023
@sequba
Copy link
Contributor Author

sequba commented Apr 4, 2024

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

@sequba sequba changed the title Fix documentation of simpleCellAddressToString and simpleCellRangeToString Make signatures of simpleCellAddressToString and simpleCellRangeToString more logical Apr 4, 2024
@sequba sequba removed the Docs Improvements or additions to documentation label Apr 4, 2024
@sequba sequba linked a pull request Nov 7, 2024 that will close this issue
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Something we can add later on without introducing a breaking change Impact: Low
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants