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) marker steps 🙀 #9364

Merged
merged 10 commits into from
Jul 31, 2023
Merged

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Jul 29, 2023

  • validate markers
  • err if markers are 'matched' but not 'emitted'
  • assumes string variables are 'emitted'
  • binary format: sets the 'vars.markers' list (and basic.txt to match)

#9119

@keymanapp-test-bot skip

- 'transform' doesn't have an 'after=' attribute

For: #9118
- tests for marker validation
- TDD: it fails, hooray

For: #9119
- wip , still not working

For: #9119
- was using the wrong syntax, hyphen instead of underscore

#9119
@srl295 srl295 added this to the A17S18 milestone Jul 29, 2023
@srl295 srl295 self-assigned this Jul 29, 2023
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Jul 29, 2023

User Test Results

Test specification and instructions

User tests are not required

@@ -27,7 +27,6 @@ Markers can appear in both 'emitting' and 'matching-only' areas:
#### Match only

- `transform from=` to match markers
- `transform after=` to match markers
Copy link
Member Author

Choose a reason for hiding this comment

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

where did this come from?

developer/src/kmc-ldml/src/compiler/vars.ts Outdated Show resolved Hide resolved
- split out MarkerTracker, could give us more precise messages about marker use
- for now, we parse all markers twice.
- update builder for the markers list

#9119
- .. and the basic.txt to prove it

#9119
- a falsy list should show up as list #0

#9119
Base automatically changed from chore/core/7375-reorder-comments-epic-ldml to master July 29, 2023 22:01
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

Comment on lines +181 to +185
validateVarsMarkers(keyboard: LDMLKeyboard.LKKeyboard, mt : MarkerTracker) : boolean {
keyboard?.variables?.string?.forEach(({value}) =>
mt.add(MarkerUse.variable, MarkerParser.allReferences(value)));
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to do validation?

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 supports validation by checking existence. Though I thought maybe there should be a second looser regex that catches invalid marker ids ?

…s-badref-0.xml

Co-authored-by: Marc Durdin <marc@durdin.net>
@srl295 srl295 merged commit 2fa9e8e into master Jul 31, 2023
2 checks passed
@srl295 srl295 deleted the feat/developer/9119-marker-epic-ldml branch July 31, 2023 20:57
@keyman-server
Copy link
Collaborator

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

3 participants