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

Only keep at most 31 characters for sheetname #831

Merged
merged 1 commit into from
Jun 3, 2019

Conversation

kaleo211
Copy link
Contributor

Addressing Issue 398.

@alubbe
Copy link
Member

alubbe commented May 23, 2019

Could you please add 2 tests for this:

  1. a 31 character name is unchanged
  2. a 32 character name is truncated

@alubbe
Copy link
Member

alubbe commented May 23, 2019

@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?

Copy link
Member

@Siemienik Siemienik left a 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:

  1. a 31 character name is unchanged
  2. a 32 character name is truncated
  1. two names with more characters than 31, where first 31 characters are equals

eq.

ThisIsAnExampleNameWith32Chars_A
ThisIsAnExampleNameWith32Chars_B
  1. 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
@kaleo211
Copy link
Contributor Author

A_ThisIsAnExampleNameWith32Chars

Thank you. I broke down those tests, hope this also works.

@guyonroche
Copy link
Collaborator

@alubbe

@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?
I don't think throwing an error is necessary - but something like a console warning would be appropriate.
This looks good - will merge soon

@guyonroche guyonroche merged commit 418ed91 into exceljs:master Jun 3, 2019
@lirantal
Copy link

@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 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants