refactor(web): generalize SearchQuotientSpur parent requirements 🚂#14987
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
30f660a to
604ee6c
Compare
604ee6c to
1f69bba
Compare
3f3382a to
1f3b029
Compare
1f69bba to
7b67bc5
Compare
1f3b029 to
9c197bd
Compare
f0978aa to
c28bb20
Compare
mcdurdin
left a comment
There was a problem hiding this comment.
I think this is okay. It's very hard to tell if there are logic changes at all. But LGTM
ec60680 to
bc23b7b
Compare
bc23b7b to
aa538c5
Compare
These changes were previously part of #14987 and have been extracted for ease of review. Build-bot: skip build:web Test-bot: skip
5adf510 to
28ded16
Compare
…nstructor This has been factorized out of #14987 for ease of review. Build-bot: skip build:web Test-bot: skip
This was formerly part of #14987 and has been extracted for ease of review. Build-bot: skip build:web Test-bot: skip
These changes were previously part of #14987 and have been extracted for ease of review. Build-bot: skip build:web Test-bot: skip
b467cc4 to
445cca2
Compare
As an upcoming goal is to introduce a new SearchQuotientNode type that will assist with context-caching across multiple tokenizaitons, it is wise to generalize SearchQuotientSpur and functions utilizing it to accept any SearchQuotientNode-implementing type as its parent. Build-bot: skip build:web Test-bot: skip
28ded16 to
3a56e23
Compare
|
I've now reworked this PR and split it into multiple segments focused on each individual "moving part". Hopefully that'll make the changes easier to follow. |
| * Designed explicitly for use in unit testing; it's not super-efficient, so | ||
| * avoid live use. |
There was a problem hiding this comment.
Given this we should probably lean towards using the unitTestEndpoints pattern?
There was a problem hiding this comment.
After a bit of thought, I opted for a different option - it's now a standalone method that is no longer part of the type.
Among other things, this also allows it to be tree-shaken easily.
There was a problem hiding this comment.
Not sure why you don't want to use the accepted pattern but this is at least better than before
There was a problem hiding this comment.
I am a bit curious how that works when declared on an interface, admittedly.
Looking at https://github.com/keymanapp/keyman/wiki/Unit-Tests#typescript, would it be best to wrap that standalone function in its own unitTestEndpoints separate from the interface?
There was a problem hiding this comment.
For interfaces, it probably doesn't work. But you could export a unitTestEndpoints-style interface that can be cast to?
For the function quotientPathHasInputs in question, rather than exporting it directly, have:
export const unitTestEndpoints = {
quotientPathHasInputs,
};
As an upcoming goal is to introduce a new SearchQuotientNode type that will assist with context-caching across multiple tokenizations, it is wise to generalize SearchQuotientSpur and functions utilizing it to accept any SearchQuotientNode-implementing type as its parent.
Build-bot: skip build:web
Test-bot: skip