feat(developer): update strs compiler to collect context #14064
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
|
wrong example, it was a tran example |
9d54d85 to
db24750
Compare
…call sites - disp, keys, layr, tran For: #13932
|
There was a problem hiding this comment.
ahhh - yes, zap and scrub have the same function. That's what I get for a long chain of PRs.
In a future PR, I remove zapMessageMetadata() and just use scrubContextFromMessages() instead.
There was a problem hiding this comment.
LGTM, although I think we need to rethink our use of x and context, because it's really ambiguous. Does compileContext or compilerContext work for you? It's really only specific to the compiler, whereas x could be anything, and context fights with the existing Keyman term.
| @@ -161,6 +173,8 @@ export interface StrsOptions { | |||
| nfd?: boolean; | |||
| /** string can be stored as a single CharStrsItem, not in strs table. */ | |||
| singleOk?: boolean; | |||
| /** optional context */ | |||
| x?: any; | |||
There was a problem hiding this comment.
Why named x? Also 'context' is a bit of an overloaded term, so I wonder if we can give a longer name, such as compileContext?
There was a problem hiding this comment.
It's x like the other parameter to m() is traditionally named o
Perhaps ObjectWithMetadata should be renamed ObjectWithCompileContext ?
There was a problem hiding this comment.
Perhaps
ObjectWithMetadatashould be renamedObjectWithCompileContext?
Yes, that sounds good to me.
It's
xlike the other parameter tom()is traditionally namedo
See #14117 (comment)
| /** add any context from the options to this strsitem */ | ||
| setContext(opts?: StrsOptions) { | ||
| // At present, there's only a single piece of context available | ||
| this._context = this._context || opts?.x; | ||
| } | ||
|
|
||
| get context() : any { | ||
| return this._context; | ||
| } |
There was a problem hiding this comment.
Can we use consistent pattern here? (either both get context()/set context(), or getContext()/setContext()? (I am not a big fan of getters/setters because they hide behaviour for what looks like a simple assignment, but that's a separate story)
There was a problem hiding this comment.
also move this to:
|
Changes in this pull request will be available for download in Keyman version 19.0.58-alpha |
For: #13932
@keymanapp-test-bot skip