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

move export code into export plugin #94

Merged
merged 2 commits into from
Aug 7, 2022
Merged

move export code into export plugin #94

merged 2 commits into from
Aug 7, 2022

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Jul 28, 2022

As proposed in python-poetry/poetry#5980 (comment): let's move code that is only used by the export plugin into the export plugin.

I've taken the liberty of rolling up a few fixes that have been stuck, probably because of the difficulty of doing the cross-repository testing. This will close all of:

and it's some sort of step in the right direction for python-poetry/poetry#5980.

I've also removed the dev-dependency on the poetry master branch:

  • the lockfile was of course pinning it to a particular sha so this wasn't doing anything to help the plugin be sure that it was compatible with master poetry
  • published 1.2.0b3 therefore seems a more sensible target.

It might be desirable for this plugin to verify that it is in fact compatible with master poetry; but I don't do that here.

Once this is merged and tagged, the corresponding code can be removed from poetry - and IMO at that point it would be sensible for the poetry pipeline to stop running the export-plugin tests.

@radoering
Copy link
Member

In general, I think it's a step into the right direction. The only reason to keep this code in poetry would be that other plugins may use it. However, I don't think this is a likely use case.

To my taste, this PR is a bit overloaded. I'd like one PR that just moves the code without any functional adjustments and subsequent PRs that incorporate the fixes. (But that's only my personal opinion. Others might be fine with one PR.)

@dimbleby
Copy link
Contributor Author

dimbleby commented Aug 5, 2022

if y'all would merge the MRs over in poetry, then that's what this would be!

@radoering
Copy link
Member

That's "fix bugs first and move code afterwards". That's a bit tricky due to the failing tests. What I proposed is "move code first and fix bugs afterwards". That should work better.

I'm just saying that it's easier to review moving the code if there are no functional changes in between. And later the commit history is more comprehensible.

@dimbleby
Copy link
Contributor Author

dimbleby commented Aug 5, 2022

Frankly I got fed up sorting out merge conflicts and rearranging test scripts across the various fixes. Compare the code in this MR to the code in python-poetry/poetry#5711 if you're worried that I might have copy-pasted wrong.

Agree that the commit history is slightly less pretty than it might be, but I decided my time was worth more than that - sorry!

radoering and others added 2 commits August 7, 2022 14:05
Co-authored-by: David Hotham <david.hotham@blueyonder.co.uk>
By treating different python version ranges independently, we buy the
flexibility needed to make better decisions.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants