Add factories for stages and scope handlers #1396
Conversation
| @@ -33,7 +36,12 @@ export function createCursorlessEngine( | |||
|
|
|||
| const testCaseRecorder = new TestCaseRecorder(hatTokenMap); | |||
|
|
|||
| const actions = new Actions(snippets, rangeUpdater); | |||
| const scopeHandlerFactory = new ScopeHandlerFactoryImpl(); | |||
There was a problem hiding this comment.
@AndreasArvidsson notice how the scopeHandlerFactory is instantiated at the composition root, so there's no "deep drilling". Anything that a scope handler needs to do its job just gets passed one level through the factory to the scope handler that needs it. The decoupling actually results in shallower hierarchies, because the code that uses scope handlers isn't responsible for giving them their dependencies. It also makes testing easier, because we could just pass in a fake scope handler factory to a modifier. Tbh it would be a bit cleaner if we could just pass in a scope handler directly to a modifier, but the problem is that we might need a different scope handler per target, so it is useful for the modifier to be able to construct the scope handler on the fly
Note that this line is the primary motivator for prioritising this PR: it allows me to easily pass in the tree-sitter parser to the TreeSitterScopeHandler I'm creating in #629
| @@ -0,0 +1,134 @@ | |||
| import { | |||
There was a problem hiding this comment.
I wonder if we want to move these factories into the composition root; one could argue they're really part of the composition root, but otoh there's something nice about having them close to all the things they instantiate. Idk wdyt @auscompgeek @AndreasArvidsson ?
pokey
force-pushed
the
pokey/add-factories-for-stages-and-scope-handlers
branch
from
abfe350 to
056a3f6
Compare
| constructor( | ||
| private modifierStageFactory: ModifierStageFactory, | ||
| private markStageFactory: MarkStageFactory, | ||
| private context: ProcessedTargetsContext, |
There was a problem hiding this comment.
Notice that putting these functions onto a class allows us to avoid passing the context around so much by just making it a member variable. We'll eventually split up / remove the context altogether, but leaving that for later
pokey
force-pushed
the
pokey/add-factories-for-stages-and-scope-handlers
branch
3 times, most recently
from
4dcc6c5 to
6f058e4
Compare
pokey
force-pushed
the
pokey/package-composition-roots
branch
from
0532bc6 to
c65c10f
Compare
pokey
force-pushed
the
pokey/add-factories-for-stages-and-scope-handlers
branch
3 times, most recently
from
aadc709 to
dc292ba
Compare
pokey
force-pushed
the
pokey/package-composition-roots
branch
from
e485803 to
20ae7c4
Compare
pokey
force-pushed
the
pokey/add-factories-for-stages-and-scope-handlers
branch
2 times, most recently
from
56b18c8 to
81d75f9
Compare
pokey
force-pushed
the
pokey/package-composition-roots
branch
from
1472b7c to
4d08e4a
Compare
pokey
force-pushed
the
pokey/add-factories-for-stages-and-scope-handlers
branch
from
81d75f9 to
b4d99c1
Compare
- Partially addresses #616 - Partially addresses #436 - Depends on #1396 ## Todo - [x] **[DISCUSS]** What to do about fallback `iterationScope`? That's the only thing that is a regression here. - [x] File issues for FIXMEs - [x] File issue for defining iteration scopes. Can probably reuse most of the code from the regular scope handler other than creating the target - [x] File issue to add unit tests for scope handlers - [x] File issue to add some Python scope types where multiple can end at the same point (due to lack of closing brackets) - [x] Add test that checks no scope types are duplicated between legacy and new definition, or file issue to add test - [x] File PR for my 7783da6 (Add support for domain, leading, trailing, interior) #1427 - [x] Look through comments on this thread for anything worth filing / doing - [x] Open as new PR? - [x] Remove extraneous test cases - [x] Double check #629 (comment); a lot of those tests we already have for the generic modifier code - [x] Make sure changes to parse-tree-extension are shipped - [x] Close #785 if we fix that - [x] Comment on #484 saying the process has started and providing link to example - [x] Close #797 if we fix that --------- Co-authored-by: Pokey Rule <[email protected]>
Checklist