Resolve all time-based snippet variables using the same time#128571
Resolve all time-based snippet variables using the same time#128571jrieken merged 1 commit intomicrosoft:mainfrom movermeyer:mo/resolve_time_based_snippet_variables
Conversation
movermeyer
changed the title
movermeyer
changed the title
jrieken
left a comment
jrieken
left a comment
There was a problem hiding this comment.
Some nit about the test, I leave it up to you. Will merge later today. Thanks so far
| const resolver = new TimeBasedVariableResolver; | ||
|
|
||
| const firstResolve = new SnippetParser().parse(snippetText).resolveVariables(resolver); | ||
| clock.tick((365 * 24 * 3600 * 1000) + (24 * 3600 * 1000) + (3661 * 1000)); // 1 year + 1 day + 1 hour + 1 minute + 1 second |
There was a problem hiding this comment.
Since _date is a new constructor argument you could simplify this further, e.g. skip clock-magic and just a fixed date when constructing the resolver
There was a problem hiding this comment.
I guess it depends on what behaviour you want to test/express.
The test as it is shows that "the same resolver will return the same values even though time has passed".
Changing the test to just use the constructor would change the test to "the resolver uses the date given".
I agree that the different tests would fail in the same ways, but IMO, the intention is captured better in the current code. 🤷
Happy to make the change if you want it.
(The resolver only has a constructor argument since we don't have "native" private fields. So think I consider it an implementation detail, and not part of the actual interface???)
There was a problem hiding this comment.
The resolver only has a constructor argument since we don't have "native" private fields.
Not sure what that means? You can define a non-native private field anywhere, not just as ctor-argument-field
There was a problem hiding this comment.
I agree that the different tests would fail in the same ways, but IMO, the intention is captured better in the current code. 🤷
Yeah, either way is fine for me. I would have done it different or most likely I wouldn't have written a test ;-)
There was a problem hiding this comment.
You can define a non-native private field anywhere, not just as ctor-argument-field
Whoops. Fixed in #128857
It was incorrectly placed there by me in microsoft#128571
It was incorrectly placed there by me in microsoft#128571
2 participants
What are you trying to accomplish?
This PR fixes #128570
What approach did you choose and why?
Since
TimeBasedVariableResolveris only used as part ofCompositeSnippetVariableResolverand immediately used to resolve all the variables in a snippet, I added the time as a field onTimeBasedVariableResolverwithout worrying about it getting stale.The impact of these changes
All the time-based snippet variables in a snippet will resolve using the same time, fixing #128570
Checklist