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

Fix issue 1474 (to check invalid sheet name) #1484

Merged

Conversation

skypesky
Copy link
Contributor

@skypesky skypesky commented Oct 2, 2020

Summary

Fix issue #1474 .
An exception is thrown when the name is illegal.

Test plan

For this I have added unit tests.

@skypesky skypesky changed the title Fix issue 174 (to check invalid sheet name) Fix issue 1474 (to check invalid sheet name) Oct 2, 2020
@papb
Copy link

papb commented Oct 2, 2020

Hello! I suggest you take a look at this, do you think it would be good to also add these other checks?

Edit: oops, the other checks already exist, I missed them, sorry about that. However I noticed your regex does not catch single quotes, was this intended?

@skypesky
Copy link
Contributor Author

skypesky commented Oct 2, 2020

hi,papb,Your method is more concise, I think it's very good.
I didn't write according to the assertvalidsheetname method before, because the validation of name in the original code did not always throw an exception. In this way, how to organize it becomes very strange.

About double quotation marks

It seems that the naming rules of sheet have something to do with the version of Excel

As for single quotation marks

As far as I know(Just now), single quotation marks can be placed in the middle, but not the head and tail. Can you test the single quotation marks for me?

spec/integration/worksheet.spec.js Outdated Show resolved Hide resolved
spec/integration/worksheet.spec.js Outdated Show resolved Hide resolved
lib/doc/workbook.js Outdated Show resolved Hide resolved
@skypesky skypesky requested a review from alubbe October 2, 2020 14:28
Copy link
Member

@alubbe alubbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor feedback around the readability/maintainability of the test case, then we're good to go!

spec/integration/worksheet.spec.js Outdated Show resolved Hide resolved
spec/integration/worksheet.spec.js Outdated Show resolved Hide resolved
Copy link
Member

@alubbe alubbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@alubbe alubbe merged commit 5ed5a90 into exceljs:master Oct 5, 2020
@skypesky skypesky deleted the fix-issue-174-(to-check-invalid-sheet-name) branch October 15, 2020 16:13
@Sczlog
Copy link

Sczlog commented Nov 6, 2023

Although here has been a long long times since this change applied.
But I suppose if replace invalid character with _ should be a better choice?
Current solution is kinda a breaking change, code running successful (but not correct?) before and will break after update to later version.

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.

4 participants