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

allow loading extensions when a trigger is loaded from below the parent's load path #49701

Merged
merged 4 commits into from
May 12, 2023

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented May 9, 2023

@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 in Manifest.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

@KristofferC KristofferC added packages Package management and loading needs tests Unit tests are required for this change and removed needs tests Unit tests are required for this change labels May 9, 2023
@KristofferC
Copy link
Member Author

I went ahead with the change to allow extensions getting loaded for the active project.
This is a natural consequence to allow once the functionality in the first commit is changed.

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.

@vtjnash
Copy link
Member

vtjnash commented May 10, 2023

I agree that sounds much more logically consistent.

@KristofferC KristofferC added the backport 1.9 Change should be backported to release-1.9 label May 11, 2023
Copy link
Member

@staticfloat staticfloat left a 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!

@KristofferC KristofferC merged commit d55314c into master May 12, 2023
@KristofferC KristofferC deleted the kc/ext_Load branch May 12, 2023 18:54
KristofferC added a commit that referenced this pull request May 15, 2023
…nt's load path (#49701)

also allow loading extensions of the active project

(cherry picked from commit d55314c)
KristofferC added a commit that referenced this pull request May 27, 2023
…nt's load path (#49701)

also allow loading extensions of the active project

(cherry picked from commit d55314c)
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label May 28, 2023
kpamnany pushed a commit that referenced this pull request Jun 21, 2023
…nt's load path (#49701)

also allow loading extensions of the active project

(cherry picked from commit d55314c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"ERROR: LoadError" when loading an extension trigger in another environment
3 participants