-
-
Notifications
You must be signed in to change notification settings - Fork 109
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(common/models): support direct-child access for Trie node iteration 📚 #11869
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nits only. Although use of USVString
could be worthy of deeper discussion.
/* | ||
Note: would otherwise return the current instance if `char == ''`. If | ||
such a call is happening, it's probably indicative of an implementation | ||
issue elsewhere - let's signal now in order to catch such stuff early. | ||
*/ | ||
if(char == '') { | ||
return undefined; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to log something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard for the mobile apps to properly log console-logging statements made from the WebWorker. I don't know that it's worth the trouble when we don't already have a pattern established for capturing said logs.
@@ -101,6 +105,38 @@ describe('Trie traversal abstractions', function() { | |||
assert.isEmpty(eKeys); | |||
}); | |||
|
|||
it('direct traversal with simple internal nodes', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description is not very fluent -- ideally, should be something that works with "it", like:
it('direct traversal with simple internal nodes', function() { | |
it('directly traverses a trie model with simple internal nodes', function() { |
But this is a nit only, I don't really care that much
…sal-probs' into feat/common/models/direct-lexicon-traversal
Changes in this pull request will be available for download in Keyman version 18.0.70-alpha |
This PR was originally part of #10973.
These changes will facilitate the next PR's changes (#11870), in which it becomes possible to support prediction, in full, solely via the
LexiconTraversal
iterator structure.Note that this gives us a neat way to support new model types: if a
LexiconTraversal
can be defined for that model, we can efficiently search it and predict for it, even if it's not actually supported by a Trie! There would be no need for the model to define a separatepredict
method on the model itself, as it can be implemented via the traversal.@keymanapp-test-bot skip