[Refactor] Introduce TranslatorContext (1/2) #638
Conversation
czechboy0
changed the title
Yeah same, but I just moved it, it's not a new name. Happy for us to rename it globally separately. |
### Motivation Depends on #638, so don't review until that lands. Continuation of #638, broke up into multiple PRs for easier review. Also a non-functional change, purely a refactoring. ### Modifications Changed a bunch of TypeMatcher methods from static to being instance methods, which gives them access to the translator context. Moved a few other methods that depended on the static methods to be extensions on the closest type that already has access to the context. ### Result Now TypeAssigner/TypeMatcher can take runtime configuration, e.g. through feature flags. ### Test Plan All tests still pass.
|
Would it make sense for a 'part 3' PR which adds the |
|
No, that was my original plan, but Context contains a lot more information than we need to propagate, which would make testing unnecessary noisy. The idea is that we copy/extract whatever we need from Config onto TranslationContext as specific properties. For example, for the UUID PR, there should be a new Bool property added on TranslationContext called |
|
Ok that makes a lot of sense, thanks. Though in the context of the UUID PR wouldn't the |
|
That's one way, but I think it's better to be very explicit about the exact inputs that are needed, rather than passing all of feature flags or the entire Config in. As that makes testing more difficult, as it's not clear which parts need to be mocked in tests and which are ignored. |
|
True, if the config or feature flags end up quite large then it would be difficult to know what needs to be mocked without a full understanding of everywhere it's used. So I guess it'll be up to the UUID PR to now set the precedent of adding a property. 👍 |
Motivation
To make upcoming PRs less noisy, we need a way to propagate configuration all the way deep to types like TypeAssigner/TypeMatcher, which historically didn't have that need.
Modifications
Introduced a new wrapper type called TranslatorContext, which wraps the existing closure that converts arbitrary strings into safe Swift names (which might in the future also need to be further customized).
The idea is that this new struct is what we'd attach more config to shortly.
There'll be a few more smaller PRs like this, I'm breaking them up for easier review.
Result
Generalized the way to propagate config into low level translator utilities.
Test Plan
All tests still pass.