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

feat(developer): range warning for non-NFD chars 🙀 #10614

Merged
merged 7 commits into from
Feb 7, 2024

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Feb 2, 2024

@srl295 srl295 self-assigned this Feb 2, 2024
@mcdurdin mcdurdin linked an issue Feb 3, 2024 that may be closed by this pull request
@mcdurdin mcdurdin modified the milestones: A17S31, B17S1 Feb 3, 2024
Comment on lines +380 to +387
protected analyzeCodePoint(c: string, ch: number): BadStringType {
const nfd = c.normalize("NFD");
if (c !== nfd) {
return BadStringType.denormalized;
} else {
return null;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder about the performance of this -- seems like it could be quite expensive?

Copy link
Member Author

Choose a reason for hiding this comment

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

right. maybe some kind of cache would help.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, could perhaps cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's only on actual ranges in transforms.

Copy link
Member

Choose a reason for hiding this comment

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

OK, limited use so we can probably ignore for now. If it's always a single codepoint we could build a map and ship it -- trivial. Danger with that is that if we use normalize() elsewhere then the map could be inconsistent wrt Unicode version.

@mcdurdin
Copy link
Member

mcdurdin commented Feb 5, 2024

Our messages (errors, hints, and warnings) don't have line numbers at present, which is going to be quite painful for users!

@srl295
Copy link
Member Author

srl295 commented Feb 5, 2024

Our messages (errors, hints, and warnings) don't have line numbers at present, which is going to be quite painful for users!

what about... << moving it to a new ticket! #10636

Base automatically changed from feat/developer/10554-sometimes-no-norm to master February 7, 2024 03:38
@srl295 srl295 merged commit 72c8988 into master Feb 7, 2024
19 checks passed
@srl295 srl295 deleted the feat/developer/10316-range-warning branch February 7, 2024 03:39
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.262-alpha

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

Successfully merging this pull request may close these issues.

feat(developer): regex: support ranges 🙀
3 participants