-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Only keep at most 31 characters for sheetname #831
Conversation
Could you please add 2 tests for this:
|
@guyonroche I think we could print a warning when this happens, or maybe even throw an error, asking the user of the library to only submit short names. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add 2 tests for this:
- a 31 character name is unchanged
- a 32 character name is truncated
- two names with more characters than 31, where first 31 characters are equals
eq.
ThisIsAnExampleNameWith32Chars_A
ThisIsAnExampleNameWith32Chars_B
- similar but with first letter different:
A_ThisIsAnExampleNameWith32Chars
B_ThisIsAnExampleNameWith32Chars
Because as far as I know, names can't be equals, what was happened in case no 3 after truncating. So it throws an error that names are equals even if they aren't. Please check it.
Also: - Throw error when create worksheet with name exists - Modify some existing tests due to over size worksheet name
Thank you. I broke down those tests, hope this also works. |
|
@guyonroche just chiming in here after I discovered now that this broke some things for me. In the future, I would consider changes like this one as a breaking change. The problem it introduced is that because it is truncating to 31 characters there is now increased potential for duplicate names which will trigger an error thrown. Regardless, keep up the great work on this library 👏 |
Addressing Issue 398.