Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Refactor] Introduce TranslatorContext (1/2) #638

Merged
merged 3 commits into from
Oct 4, 2024

Conversation

czechboy0
Copy link
Contributor

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.

@czechboy0 czechboy0 changed the title [Refactor] Introduce TranslatorContext [Refactor] Introduce TranslatorContext (1/2) Oct 1, 2024
Copy link
Collaborator

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice patch. I don't love threading context around but can see the value. Don't love the name of the closure as I thought we prefer to avoid as methods in API design, but since it's not public API I'm not gonna bikeshed it 👍

@czechboy0
Copy link
Contributor Author

Don't love the name of the closure

Yeah same, but I just moved it, it's not a new name. Happy for us to rename it globally separately.

@czechboy0 czechboy0 enabled auto-merge (squash) October 4, 2024 13:29
@czechboy0 czechboy0 merged commit 07ee940 into apple:main Oct 4, 2024
24 checks passed
@czechboy0 czechboy0 deleted the hd-translator-context branch October 4, 2024 13:52
czechboy0 added a commit that referenced this pull request Oct 4, 2024
### 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.
@theoriginalbit
Copy link
Contributor

Would it make sense for a 'part 3' PR which adds the Config instance to this context? I think it would further help reduce the noise in PRs like #627 which needs to access a value on config at that same deep TypeMatcher type.

@czechboy0
Copy link
Contributor Author

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 generateTypesafeUUID or similar.

@theoriginalbit
Copy link
Contributor

Ok that makes a lot of sense, thanks.

Though in the context of the UUID PR wouldn't the featureFlags property be more desirable to copy over to avoid the noise of construction when N other similar PRs pop up and want to have their own properties too?

@czechboy0
Copy link
Contributor Author

czechboy0 commented Oct 6, 2024

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.

@theoriginalbit
Copy link
Contributor

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. 👍

@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants