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(core): ldml tertiary reordering 🙀 #9962

Merged
merged 8 commits into from
Nov 22, 2023
Merged

feat(core): ldml tertiary reordering 🙀 #9962

merged 8 commits into from
Nov 22, 2023

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Nov 8, 2023

  • implemented tertiary reordering
  • reinstated bengali reordering!
  • fixed reordering to be longest-first (by sorting the list before applying)
  • updated debug logging

Fixes: #9707

@keymanapp-test-bot skip

@srl295 srl295 self-assigned this Nov 8, 2023
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Nov 8, 2023 •
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 A17S25 milestone Nov 8, 2023
@github-actions github-actions bot added common/ common/resources/ Build infrastructure core/ Keyman Core labels Nov 8, 2023
@srl295 srl295 changed the title Feat/core/9707-tertiary-reorder-epic-ldml feat(core): ldml tertiary reordering 🙀 Nov 8, 2023
@srl295 srl295 linked an issue Nov 8, 2023 that may be closed by this pull request
feat(core): tertiary reorders 🙀 #9707
Closed
@ermshiperete
Copy link
Contributor

I'm not sure why this build failed to trigger all builds (or rather why it worked for the other PRs). Anyways, a fix is in master, so if you merge you should get a complete set of builds.

@srl295
Copy link
Member Author

srl295 commented Nov 8, 2023

I'm not sure why this build failed to trigger all builds (or rather why it worked for the other PRs). Anyways, a fix is in master, so if you merge you should get a complete set of builds.

thank you, i have updated it now

@github-actions github-actions bot added the feat label Nov 8, 2023
@mcdurdin
Copy link
Member

Keyman Core - macOS:

84/84 keyman_core:ldml / test_transforms                       FAIL            0.63s   killed by signal 6 SIGABRT

Keyman Core - WASM:

84/84 keyman_core:ldml / test_transforms                       FAIL            0.18s   exit status 7

Keyman Core - Linux:

84/84 keyman_core:ldml / test_transforms                       FAIL            0.46s   killed by signal 6 SIGABRT

Keyman Core - Windows:

03:47:15   ../../../src/debuglog.cpp(467): error C2220: the following warning is treated as an error
03:47:15   ../../../src/debuglog.cpp(467): warning C4477: 'snprintf' : format string '%4.6X' requires an argument of type 'unsigned int', but variadic argument 1 has type 'char32_t'

Keyman - Windows and Keyman - Developer are probably same as above.

Hesitant to start review with so many build errors

@mcdurdin mcdurdin modified the milestones: A17S25, A17S26 Nov 13, 2023
@mcdurdin
Copy link
Member

Hope you don't mind, will return this to draft while we wait for parent branches to stabilize and builds to pass, so it exits the review queue.

@mcdurdin mcdurdin marked this pull request as draft November 14, 2023 08:09
@srl295 srl295 force-pushed the feat/core/9450-backspace-epic-ldml branch from ce45b15 to e9e5665 Compare November 16, 2023 18:27
@srl295
feat(core): ldml tertiary reorder 🙀
85a5afa 
- failing test cases, hooray! In XML and C++

For: #9707
@srl295
feat(core): ldml tertiary reorder 🙀
f7566c7 
- implemented tertiary reordering
- reinstated bengali reordering!
- fixed reordering to be longest-first (by sorting the list before applying)
- updated debug logging

Fixes: #9707
@srl295 srl295 force-pushed the feat/core/9707-tertiary-reorder-epic-ldml branch from beb54bf to f7566c7 Compare November 16, 2023 18:31
@srl295
Copy link
Member Author

srl295 commented Nov 16, 2023

rebased

@srl295
feat(core): ldml dx: launch ldml unit tests before keyboard tests 🙀
30d951e 
For: #9707
@srl295
feat(core): ldml tertiary reorder 🙀
3cd6c76 
- handle case where there's no tertiary base before the first char
- separate some element API tests from the actual tertiary test.  Tests were failing because of now-correct implementation.

Fixes: #9707
@srl295
Merge branch 'feat/core/9450-backspace-epic-ldml' into feat/core/9707…
af3f128 
…-tertiary-reorder-epic-ldml
@srl295
Merge branch 'feat/core/9450-backspace-epic-ldml' into feat/core/9707…
3e33abf 
…-tertiary-reorder-epic-ldml
@srl295 srl295 marked this pull request as ready for review November 16, 2023 21:38
@srl295 srl295 marked this pull request as draft November 16, 2023 22:18
@srl295 srl295 marked this pull request as ready for review November 17, 2023 04:15
mcdurdin
mcdurdin approved these changes Nov 19, 2023
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 as far as I can tell! Tertiary reorders are fun I think?

Base automatically changed from feat/core/9450-backspace-epic-ldml to master November 21, 2023 22:43
@srl295 srl295 merged commit 6f4def8 into master Nov 22, 2023
23 of 25 checks passed
@srl295 srl295 deleted the feat/core/9707-tertiary-reorder-epic-ldml branch November 22, 2023 00:22
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.215-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

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

@rc-swag rc-swag Awaiting requested review from rc-swag rc-swag is a code owner

Assignees

@srl295 srl295

Labels
common/resources/ Build infrastructure common/ core/ Keyman Core feat
Projects
None yet
Milestone
A17S26
Development

Successfully merging this pull request may close these issues.

feat(core): tertiary reorders 🙀
4 participants
@srl295 @ermshiperete @mcdurdin @keyman-server