-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
allow loading extensions when a trigger is loaded from below the parent's load path #49701
Conversation
I went ahead with the change to allow extensions getting loaded for the active project. So as an explicit example, if you have PGFPlotsX as your current active project, and then load Colors from the global environment, the extension for Colors in PGFPlotsX will now load. I think this is what people naturally expected anyway. |
I agree that sounds much more logically consistent. |
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.
Works for me on my workload!
@topolarity has convinced me that this makes sense.
As it is right now, an extension gets loaded only if the trigger is above (or at the same place) as the parent in the load path.
This is kind of a symmetry break because, from the extension's point of view, there isn't really any difference between the parent and one of the triggers. Both of these are just dependencies.
The extension could be shifted from the parent to the trigger without any change of functionality.
So then why does code loading differ between these two cases?
With this PR, the extension will load no matter the relationship of load paths between the parent and triggers.
This also avoids the complication in #49681.
cc @vtjnash, @topolarity
Also, a TODO is that extensions should probably be loadable if the parent is the active project (which would need to add some code to look up extensions in Project.toml files, right now it only looks inManifest.toml
). Fortunately, I envisioned maybe having to do this so I saved some code I ripped out earlier that implements this: https://gist.github.com/KristofferC/e5c9e80db0f056ab1f72714c46477a02.^Done
Fixes #49656