-
Notifications
You must be signed in to change notification settings - Fork 887
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
defer config.add_translation_dirs and push/pop threadlocals during deferred actions/discriminators #2873
Conversation
efe34f8
to
6a4168b
Compare
I will amend this PR tonight to have |
6a4168b
to
e38c247
Compare
e38c247
to
0c1c587
Compare
Ok I upped the ante and am now calling begin/end during every commit cycle so any deferred action can depend on threadlocals. I think this makes a lot of sense. This is ready for review. |
90ad0c4
to
9c2c864
Compare
I'm going to merge this tonight unless someone reviews it by then and has an issue. It has global changes to the configurator so I was hoping there might be a review, but I've also added enough tests that I'm pretty confident in it. |
Will review tonight. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
||
- Asset specifications for directories passed to | ||
``config.add_translation_dirs`` now support overriding the entire asset | ||
specification, including the folder name. Previously on the package name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/on/only/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks
intr['spec'] = spec | ||
introspectables.append(intr) | ||
directories.append(directory) | ||
resolver = AssetResolver(self.package_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup!
9c2c864
to
5f4649e
Compare
Looks good. Thank you for fixing this.
It would be good to make title for this pull reflect broader scope of the change. |
This resolves #2046.
Added docs about importance of
config.begin()
when invokingadd_translation_dirs
.Deferred the computation of the specs until the default config phase in order to allow asset overrides defined later to have an effect.
Used the
AssetResolver
to resolve the entire spec instead of just the package name such that translation directories can override each other even if they don't have the same name / subpath.config.override_asset
now occurs in an earlier config phase to ensure assets are updated prior toadd_translation_dirs
executing.I would appreciate it if @piotr-dobrogost could review this.