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.

Sign up for GitHub

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

Jump to bottom

feat(developer,common): verify normalization of strings 🙀 #12748

Merged
merged 6 commits into from
Dec 4, 2024
Merged

feat(developer,common): verify normalization of strings 🙀 #12748

merged 6 commits into from
Dec 4, 2024

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Nov 29, 2024 •
edited
Loading

common:

  • isDenormalized() function for checking strings that are neither NFC nor NFD
  • shift the 'strs' processing slightly, because otherwise we normalize to NFD before even tracking the strings!

developer:

Fixes: #7394

@keymanapp-test-bot skip

@srl295
feat(common): isDenormalized() function for checking strings that are…
8e19508 
… neither NFC nor NFD

Fixes: #7394
@srl295
feat(common,developer): tests and warning on denormalized content
add502e 
- common: shift the 'strs' processing slightly, because otherwise we normalize to NFD before even tracking the strings
- if any string is neither NFC nor NFD, give a warning
- add tests for the same

Fixes: #7394
@srl295 srl295 self-assigned this Nov 29, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Nov 29, 2024 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S16 milestone Nov 29, 2024
mcdurdin
mcdurdin requested changes Nov 30, 2024
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

I think we should report on all the denormalized strings (nothing worse than fix/compile/rinse/repeat because the compiler won't tell you more than one problem at a time!)

common/web/types/src/util/util.ts Outdated Show resolved Hide resolved
common/web/types/src/kmx/kmx-plus/kmx-plus.ts Outdated Show resolved Hide resolved
common/web/types/src/kmx/kmx-plus/kmx-plus.ts Outdated Show resolved Hide resolved
developer/src/kmc-ldml/src/compiler/empty-compiler.ts Outdated Show resolved Hide resolved
developer/src/kmc-ldml/src/compiler/ldml-compiler-messages.ts Outdated Show resolved Hide resolved
@srl295
Copy link
Member Author

srl295 commented Nov 30, 2024

I think we should report on all the denormalized strings (nothing worse than fix/compile/rinse/repeat because the compiler won't tell you more than one problem at a time!)

Will do.

srl295 and others added 2 commits November 29, 2024 22:13
@srl295 @mcdurdin
Apply suggestions from code review
f4b7e0a 
Co-authored-by: Marc Durdin <[email protected]>
@srl295
feat(common,developer): tests and warning on denormalized content per…
6f2a6b2 
… review comments

- report a warning on each separate instance of a denormalized string (once per string).

Fixes: #7394
@srl295 srl295 requested a review from mcdurdin December 2, 2024 17:36
mcdurdin
mcdurdin approved these changes Dec 3, 2024
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM

@srl295 srl295 merged commit 41f47b8 into master Dec 4, 2024
21 checks passed
@srl295 srl295 deleted the feat/developer/7394-verify-normalization-epic-ldml branch December 4, 2024 22:58
@keyman-server
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdurdin mcdurdin mcdurdin approved these changes

@darcywong00 darcywong00 Awaiting requested review from darcywong00 darcywong00 is a code owner

@SabineSIL SabineSIL Awaiting requested review from SabineSIL SabineSIL is a code owner

@markcsinclair markcsinclair Awaiting requested review from markcsinclair markcsinclair is a code owner

@jahorton jahorton Awaiting requested review from jahorton jahorton is a code owner

Assignees

@srl295 srl295

Labels
common/web/ common/ developer/compilers/ developer/ feat
Projects
Keyman
Archived in project
Milestone
A18S16
Development

Successfully merging this pull request may close these issues.

feat(developer): verify normalization of ldml keyboard xml 🙀
3 participants
@srl295 @keyman-server @mcdurdin