Skip to content

Conversation

orgads
Copy link
Contributor

@orgads orgads commented Nov 8, 2019

Purpose / Goal

Validator: Fix name regexp

The previous implementation only allowed the "localeRange" for the first character, and it did not validate the following characters at all, because the regexp did not include $, so if the first character
matched, the regexp would match.

Replace the regexp with one that matches the XML spec (except [\u10000-\uEFFFF] which matches digits for some reason...) and match all the characters.

Remove the localeRange option, which is no longer needed

Type

Please mention the type of PR

  • [X]Bug Fix
  • [ ]Refactoring / Technology upgrade
  • [ ]New Feature

@orgads orgads changed the title Name regexp Validator: Fix name regexp Nov 8, 2019
@coveralls
Copy link

coveralls commented Nov 8, 2019

Coverage Status

Coverage decreased (-0.02%) to 96.405% when pulling 7c599bc on orgads:name-regexp into df830dc on NaturalIntelligence:master.

@amitguptagwl
Copy link
Member

localeRange option is aligned with XML to JSON parser so that if someone parse the js object back to XML, he should get the same output. Hence, we can't remove it. Please let me know what are the bugs that you see. Currently, if localrange is specified then only that certain characters should be allowed.

@amitguptagwl
Copy link
Member

Please check my latest commit

@orgads
Copy link
Contributor Author

orgads commented Nov 11, 2019

It solves the issues I found. Thank you! Can you please release a new version so we can use it?

I'm still not sure why you need localeRange. Both XML and JSON support unicode characters in names. Why don't you just allow any supported unicode the spec defines? It can be converted both ways.

@amitguptagwl
Copy link
Member

Yes, you're right. We can allow that. But in this case we'll have to remove localerange from parser and validator both.

@orgads
Copy link
Contributor Author

orgads commented Nov 12, 2019

Done. Please review.

@amitguptagwl
Copy link
Member

You're so quick. Thanks. Let me check.

As this will be a change in the API, I'll have to publish it with minor version change. I'll try to do that ASAP.

@orgads
Copy link
Contributor Author

orgads commented Nov 14, 2019

Please also update the Readme and release notes.

@amitguptagwl
Copy link
Member

I'll try to review the changes tomorrow morning :) Thanks for your effort

Copy link
Member

@amitguptagwl amitguptagwl left a comment

Choose a reason for hiding this comment

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

Please check my comments. We can probably simplify the logic and add unit test to cross verify it.

@amitguptagwl
Copy link
Member

@orgads are you still on this?

@orgads orgads force-pushed the name-regexp branch 3 times, most recently from c74c513 to 7f4881b Compare December 8, 2019 06:37
@orgads
Copy link
Contributor Author

orgads commented Dec 8, 2019

I removed localeRange from README.md and index.html, but did not remove it from parser.d.ts in order to not break existing code that uses it.

@amitguptagwl
Copy link
Member

Changes in parser.d.ts should not break anything. It'll just give syntax errors to the existing users. Anyhow, we can't release these changes under the minor version. So the changes to parser.d.ts would be fine.

Replace the regexp with one that matches the XML spec[1]
(except [\u10000-\uEFFFF] which matches digits for some reason...).

Remove the localeRange option, which is no longer needed

[1] https://www.w3.org/TR/xml/#NT-NameStartChar
@orgads
Copy link
Contributor Author

orgads commented Dec 15, 2019

The failed checks are false positives.

@amitguptagwl
Copy link
Member

Sorry for the delay. But I'm unable to take any action for next few weeks. Will update you. And thanks for your effort.

@amitguptagwl
Copy link
Member

I have cross verified for the performance. It looks fine. Let me check for other aspects. I'll update you if there is any issue.

Copy link
Member

@amitguptagwl amitguptagwl left a comment

Choose a reason for hiding this comment

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

regex needs to be checked again. It'll change the position of captured text as well. But since any test didn't fail because of this, we can add a possible test to fail it with current regex and then pass for updated regex.

@amitguptagwl amitguptagwl merged commit 2032c19 into NaturalIntelligence:master Jan 2, 2020
@orgads orgads deleted the name-regexp branch January 2, 2020 03:52
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.

3 participants