Define contextual variables exposed by VariablesAndExpressionParser using a static map in all cases, not just for the ThisLocation-related use case#4225
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (19)
📝 WalkthroughWalkthroughThe PR replaces override-based parser creation with explicit factories (standalone, control, surface), refactors contextual this:* resolution into typed context maps, updates all caller sites to the new APIs/signatures, and updates tests to assert the new contextual behavior. ChangesVariable parsing refactor across runtime flows
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jswalden
changed the title
VariablesAndExpressionParser using a static map in all cases, not just for the ThisLocation-related use case
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
companion/test/Variables/Values.test.ts (1)
5-5:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the stale import/export contract for location variable helper
Thanks for expanding the raw
this:*coverage — nice test direction. Right now, Line 5 importsInjectedVariablesForLocation, but CI shows../../lib/Variables/Values.jsno longer exports it, so type-check and tests fail before execution. Please align this test with the current exported API (or re-export the helper fromValues.ts) so the suite compiles again.Source: Pipeline failures
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ebb3599-6863-4645-a50f-c00d2263fd14
📒 Files selected for processing (31)
companion/lib/Controls/ControlStore.tscompanion/lib/Controls/ControlTypes/Button/Base.tscompanion/lib/Controls/ControlTypes/Button/Layered.tscompanion/lib/Controls/ControlTypes/Button/Preset.tscompanion/lib/Controls/ControlTypes/PageButton.tscompanion/lib/Controls/Controller.tscompanion/lib/Controls/Entities/EntityListPoolBase.tscompanion/lib/Controls/Entities/EntityListPoolButton.tscompanion/lib/Controls/IControlStore.tscompanion/lib/Graphics/ConvertGraphicsElements.tscompanion/lib/ImportExport/Backups.tscompanion/lib/ImportExport/Controller.tscompanion/lib/ImportExport/Export.tscompanion/lib/Instance/Connection/ChildHandlerLegacy.tscompanion/lib/Instance/Connection/ChildHandlerNew.tscompanion/lib/Instance/Connection/EntityManager.tscompanion/lib/Internal/Controller.tscompanion/lib/Preview/ElementStream.tscompanion/lib/Preview/ExpressionStream.tscompanion/lib/Preview/Graphics.tscompanion/lib/Surface/Controller.tscompanion/lib/Surface/IP/Satellite.tscompanion/lib/Surface/PluginPanel.tscompanion/lib/Surface/Types.tscompanion/lib/Variables/Util.tscompanion/lib/Variables/Values.tscompanion/lib/Variables/VariablesAndExpressionParser.tscompanion/test/Variables/Values.test.tscompanion/test/Variables/VariablesAndExpressionParser.test.tscompanion/test/Variables/executeExpression.test.tscompanion/test/Variables/parse.test.ts
jswalden
force-pushed
the
contextual-variables
branch
from
4b96a68 to
17c2ad4
Compare
jswalden
force-pushed
the
contextual-variables
branch
from
17c2ad4 to
f3ddc2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
companion/test/Variables/VariablesAndExpressionParser.test.ts (1)
82-104: ⚡ Quick winConsider adding one direct test for
forControl(..., surfaceId)branch.You already cover
forControl(without surface id) andforSurface, which is great. A small additional case forforControlwith a definedsurfaceIdwould lock in the dedicated branch that usesThisLocationThroughSurfaceVariables.[Suggestion only — this can be a follow-up if you prefer.]
Also applies to: 161-202
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf77883d-630e-49c5-baa0-072986c218b7
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
companion/lib/Controls/ControlStore.tscompanion/lib/Controls/ControlTypes/Button/Base.tscompanion/lib/Controls/ControlTypes/Button/Layered.tscompanion/lib/Controls/ControlTypes/Button/Preset.tscompanion/lib/Controls/ControlTypes/PageButton.tscompanion/lib/Controls/Controller.tscompanion/lib/Controls/Entities/EntityListPoolBase.tscompanion/lib/Controls/IControlStore.tscompanion/lib/ImportExport/Backups.tscompanion/lib/ImportExport/Controller.tscompanion/lib/ImportExport/Export.tscompanion/lib/Instance/Connection/ChildHandlerLegacy.tscompanion/lib/Instance/Connection/ChildHandlerNew.tscompanion/lib/Instance/Connection/EntityManager.tscompanion/lib/Internal/Controller.tscompanion/lib/Surface/Controller.tscompanion/lib/Variables/Values.tscompanion/lib/Variables/VariablesAndExpressionParser.tscompanion/test/Instance/EntityManager.test.tscompanion/test/Variables/Values.test.tscompanion/test/Variables/VariablesAndExpressionParser.test.tspackage.json
✅ Files skipped from review due to trivial changes (2)
- package.json
- companion/test/Instance/EntityManager.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- companion/lib/Instance/Connection/EntityManager.ts
- companion/lib/ImportExport/Backups.ts
- companion/lib/Controls/ControlTypes/Button/Preset.ts
- companion/lib/Instance/Connection/ChildHandlerNew.ts
- companion/lib/Controls/ControlTypes/Button/Layered.ts
- companion/lib/Instance/Connection/ChildHandlerLegacy.ts
- companion/test/Variables/Values.test.ts
- companion/lib/Internal/Controller.ts
- companion/lib/Controls/Controller.ts
- companion/lib/Variables/Values.ts
- companion/lib/Controls/ControlStore.ts
- companion/lib/Variables/VariablesAndExpressionParser.ts
jswalden
force-pushed
the
contextual-variables
branch
from
f3ddc2a to
fbadae9
Compare
jswalden
force-pushed
the
contextual-variables
branch
from
fbadae9 to
caa54b9
Compare
|
Heh, I see #4234 is afoot. Guess that'll be another case of overriding to do some minor work shunting through this, once it's completed. |
|
yeah sorry about that. It might be one minor merge conflict though? (unless you also want to move some of the new code I have added?) |
|
Yeah, the new code I think ought be transitioned over. Shouldn't be hard to do. No worries about mild merge conflicts, it comes with the terrain working on shared code. |
|
I am wondering if the right approach is to be pushing all the 'overrides' so that VariablesAndExpressionParser owns the definitions of these variations, or whether it is better for the places which use these parsers to bring this knowledge. But I'm wondering that when I come to add more types of controls (with differing supported this variables), should it be the VariablesAndExpressionParser that has to know about this, or it feels like it would be cleaner (from a modularity view, less clean in some other ways) to have that knowledge be held by the type of the control (which wasnt in place to handle at all). I am aware that this is very much something that doesnt have a right and a wrong answer, so this is really just a gentle question and giving myself some time to see if I form a stronger feeling either way. |
|
Allowing users to bring their own static variables set seems pretty reasonable. Something like (with the necessary typing added to force vars/context type compatibility) class BringYourOwnVars {
vars
context
constructor(vars, context) {
this.vars = vars
this.context = context
}
has(id) { return id in this.vars }
get(id) { return id in this.vars ? this.vars[id](this.context) : undefined }
}would let a user pass in their statically defined record along the lines of the couple static sets in this patch, without the parser having to know all the details. I like the idea of keeping some clear delineated separation between statically enumerable variables, and the arbitrary variables provided by |
|
Okay, I'm trying to merge all this...and also trying to figure out how to conceptualize the new stuff and the existing stuff. You're right, they're kind of skew on the purpose front. My aim here was to make at least somewhat clear, semi-statically, what variables are incorporated into each created parser -- and to eliminate per-contextual-variable up-front cost. This is somewhat motivated by the It would make sense to build Perhaps the lowest-level parser construction path should take 1) a |
Honestly I am not sure what the approach should be here. I understand what you are saying, but am struggling to get into the space to figure out what all the routes are and how to tidy things up. So I dont have any helpful input on the approach to take at this point. This isnt the first time that the ui and backend have drifted on which special variables they support, so I do agree it seems like a weak point that would be good to address somehow. A thought that could help on the ui side when you get to that; if the list of variables does belong to the control, the control could report these same list to the ui in its data blob. This could include a bunch of flags (eg 'excludeFromStoreResult') and could be thrown into a react context, or maybe just passed as a prop to bubble it through the hierarchy and lets the usages pick it up. I am not set on it necessarily being driven by the control though, thats just feels like the natural ownership to me currently.
I think this target forwarding is a bit of a special case, I cant think of where else we might want something like this at the moment. |
2 participants
Only the final rev is new here -- the rest are just #4216 -- but Github doesn't seem to have a way to encode that and only show the final rev, that I know of.
This changes parser handling up enough that all statically injected variables are now enumerated and computed from a static map object, similar to what
ThisLocationVariablescurrently does.VariablesAndExpressionParserloses its#thisValuesmap and instead gains 1) a#contextDatacontaining stuff like control location, surface ID, etc. as needed for all defined statically injected variables, and 2) a#contextVariablesmap of all statically injected variables' names (for the particular variables flavor being implemented) to functions computing their values from that#contextDatavalue.Presently override variable values are used for exactly two things: 1) to inject
this:surface_idinto internal actions' variable parsing, and 2) per-graphical-element property overrides increateParseElementsContext. This patch changes the first case into a statically injected variable and removes override variable values from the parser interface entirely -- except throughcreateChildParser. Given that overriding is the exceptional case, I think it is a good thing to impose that interface complexity on only the single caller that wants it -- and to make it easier to find the complexity by routing variable overriding through one call signature easily searched for and downstream uses readily tracked.A bunch of the new function names are more or less just placeholder names so I could keep moving forward, with very little thought given to them. Feel free to suggest better names.
createStandaloneParseris called in once place withundefinedsurfaceIdand nonnulllocalValues, in one place with non-undefinedsurfaceIdand nulllocalValues, and all other places withundefined/nullfor them. The first two could perhaps be split into separately named functions, and the rest left in a no-argument function, with correspondingly reduced statically-injected contextual variables, if that made things clearer.Summary by CodeRabbit
Refactor
Tests
Chores