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): add sample file, update union #5015 🙀 #7071

Merged
merged 5 commits into from
Aug 16, 2022
Merged

feat(core): add sample file, update union #5015 🙀 #7071

merged 5 commits into from
Aug 16, 2022

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Aug 13, 2022 •
edited
Loading

  • add tiny sample kmx
  • update union
  • add DTD

#5015

@keymanapp-test-bot skip

@srl295 srl295 self-assigned this Aug 13, 2022
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Aug 13, 2022
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Aug 13, 2022 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

mcdurdin
mcdurdin approved these changes Aug 15, 2022
View reviewed changes
core/src/kmx/kmx_file.h Outdated Show resolved Hide resolved
core/tests/unit/ldml/001_tiny.xml Outdated

<keys>
<key id="hmaqtua" to="ħ" />
<key id="say" to="ថា" />
Copy link
Member

Choose a reason for hiding this comment

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

Technically, that rather than say but very good 😆

core/tests/unit/ldml/ldmlKeyboard.dtd Outdated
@@ -0,0 +1,192 @@
<!--
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 if we can put this in resources/standards-data/ldml-keyboard along with a README.md detailing where it is sourced from and update strategy?

See for example https://github.com/keymanapp/keyman/blob/d4b8094b137ef2a1aac1eb60a6d0a01658bb525f/resources/standards-data/langtags/readme.md and its corresponding data file.

I realise that this may be problematic/annoying for DOCTYPE decls so feel free to push back on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's reasonable. Will need a long term strategy for this anyway. Should be versioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/keymanapp/keyman/blob/master/resources/standards-data/unicode-character-database/readme.md this one is not so descriptive :)

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I love the simplicity of that readme.

@srl295 srl295 requested a review from darcywong00 as a code owner August 15, 2022 16:53
@srl295 srl295 changed the title feat(core): add sample file, update union 🙀 feat(core): add sample file, update union #5015 🙀 Aug 15, 2022
@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label Aug 15, 2022
@github-actions github-actions bot added the common/resources/ Build infrastructure label Aug 15, 2022
@srl295
Copy link
Member Author

srl295 commented Aug 15, 2022

00041d6 belongs in PR #7066 instead

@srl295 srl295 mentioned this pull request Aug 15, 2022
Merged
@srl295 srl295 force-pushed the feat/core/5015-process0 branch from 0a2167d to 4f141eb Compare August 15, 2022 18:39
@srl295
Copy link
Member Author

srl295 commented Aug 15, 2022

👉 Note: rebased to pickup upstream PR changes

mcdurdin
mcdurdin reviewed Aug 16, 2022
View reviewed changes
core/src/kmx/kmx_file.h Outdated Show resolved Hide resolved
Base automatically changed from feat/core/7046-processflag to feature-ldml August 16, 2022 13:10
srl295 and others added 5 commits August 16, 2022 10:24
@srl295
test(core): add sample tiny file
29d4cc0 
- also snapshot dtd
@srl295
feat(core): update COMP_KEYBOARD_EX #5015
189124b 
- uberstruct
@srl295 @mcdurdin
Apply suggestions from code review
1ccc43d 
Co-authored-by: Marc Durdin <[email protected]>
@srl295
feat(core): Sample LDML data updates, kmx_file #5015
9e44a7f 
Review comments:
- moved KMXPLUSINFO into separate struct
- created resource/standards-data/ldml-keyboards for CLDR data
- corrected Khmer err
@srl295 @mcdurdin
Update core/src/kmx/kmx_file.h
5e203f8 
Co-authored-by: Marc Durdin <[email protected]>
@srl295 srl295 force-pushed the feat/core/5015-process0 branch from 7fa78a1 to 5e203f8 Compare August 16, 2022 15:25
@srl295
Copy link
Member Author

srl295 commented Aug 16, 2022

just a rebase onto the feature branch

@srl295
Copy link
Member Author

srl295 commented Aug 16, 2022

What is the modelling layer error?

LMLayer using the trie model Regression tests.Chrome 100.0.4896.127 (Windows 10):

@mcdurdin
Copy link
Member

What is the modelling layer error?

Lexical Models. You can ignore it. Being addressed by #7037.

@srl295 srl295 merged commit 3c7279d into feature-ldml Aug 16, 2022
@srl295 srl295 deleted the feat/core/5015-process0 branch August 16, 2022 16:13
@mcdurdin mcdurdin added this to the Support milestone Aug 18, 2023
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

@darcywong00 darcywong00 Awaiting requested review from darcywong00

Assignees

@srl295 srl295

Labels
common/resources/ Build infrastructure common/ developer/ epic-ldml feat
Projects
None yet
Milestone
Support
Development

Successfully merging this pull request may close these issues.
2 participants
@srl295 @mcdurdin