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

remove circular dependency on export plugin #5980

Merged
merged 4 commits into from
Oct 5, 2024

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Jul 9, 2022

The circular dependency has been bothering me for a while, but #6441 was the nudge to do something about it.

poetry should not depend on the plugin: the plugin should depend on poetry.

This means that docs that talk about poetry export arguably don't belong here at all: but mostly they already say "this is actually provided by poetry-plugin-export" so I've just beefed that up slightly.

I'm about to find out what I've broken in the github workflows by removing this dependency...

@finswimmer
Copy link
Member

I'm courious how this should work. IIRC removing the plugin essentially means removing the export command from the default installation. This is something we really like to do in the future, but for now this would be a breaking change.

@dimbleby
Copy link
Contributor Author

dimbleby commented Jul 9, 2022

Sure, people who want poetry export will need to install the plugin themselves.

If we want poetry export to be a default command: then move it back in-tree in this repository. But if we want it to be a plugin, then let's treat it as a plugin.

@dimbleby
Copy link
Contributor Author

I can see we're going to need consensus here... somehow.

While we're thinking about disentangling these two projects, here are some more thoughts.

I'd be inclined to transfer the bits of export-specific code that linger in this repository over to the plugin entirely. That is: get_project_dependency_packages(), and its children in the call graph.

That would allow deleting the horrid cross-repository github workflow, making it much easier to make fixes to the export function.

It would also make it easier to add enhancements if the export plugin was more involved in walking the dependency tree eg python-poetry/poetry-plugin-export#91 would like access to information that is known inside that code.

To be clear, I don't intend to do any of that any time soon. But this seems as good a place as any to dump some notes.

@dimbleby dimbleby force-pushed the no-export-plugin branch 4 times, most recently from b1a1bc3 to 2f4a1ff Compare March 29, 2024 20:34
@xylar
Copy link

xylar commented Apr 16, 2024

I'm still in favor of eliminating this circular dependency but I just wanted to point to my comment here:
#6441 (comment)
It seems we can handle circular dependencies in conda packages, it's just a little awkward.

@dimbleby dimbleby force-pushed the no-export-plugin branch 3 times, most recently from 0e974c9 to caa633c Compare May 25, 2024 12:25
@Secrus Secrus linked an issue Sep 26, 2024 that may be closed by this pull request
@radoering radoering added impact/docs Contains or requires documentation changes and removed status/needs-consensus Consensus among maintainers required labels Oct 3, 2024
Copy link

github-actions bot commented Oct 3, 2024

Deploy preview for website ready!

✅ Preview
https://website-brvu0ng7v-python-poetry.vercel.app

Built with commit 7ff9284.
This pull request is being automatically deployed with vercel-action

@radoering
Copy link
Member

After having merged #9547, I think there is consensus that we can move on here.

docs/cli.md Outdated Show resolved Hide resolved
@dimbleby
Copy link
Contributor Author

dimbleby commented Oct 3, 2024

I've rebased, am happy to accept pull requests on my fork with tweaks / allow maintainers to push / have someone else follow up separately with further change

@radoering radoering enabled auto-merge (squash) October 5, 2024 11:31
@radoering radoering merged commit 09d2e50 into python-poetry:main Oct 5, 2024
76 checks passed
@dimbleby dimbleby deleted the no-export-plugin branch October 5, 2024 11:58
Secrus pushed a commit to Secrus/poetry that referenced this pull request Oct 27, 2024
Copy link

github-actions bot commented Nov 5, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
impact/docs Contains or requires documentation changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cyclic dependency between poetry and poetry-plugin-export