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

Drop lockfile as direct dependency #7310

Closed
wants to merge 1 commit into from

Conversation

Secrus
Copy link
Member

@Secrus Secrus commented Jan 6, 2023

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.

Direct dependency on lockfile wasn't really necessary, I have brought all the necessary code into one class building upon what was done in #6471

@dimbleby
Copy link
Contributor

dimbleby commented Jan 6, 2023

What's the motivation?

What I mean is: given that cachecontrol is pulling in the lockfile dependency anyway, does this buy anything? Or does it just take some messy code that we're indirectly using anyway, and duplicate it but make it also poetry's responsibility?

If and when cachecontrol ever (re)releases the version that includes my MR to drop lockfile, the code here should be removed altogether. But I'm not seeing a future in which poetry does have this code, and also can actually eliminate lockfile from the dependency tree.

@Secrus
Copy link
Member Author

Secrus commented Jan 6, 2023

Actually, I was looking into the cachecontrol code, and having this, we could hack the lockfile extra away, since cachecontrol only uses lockfile if lock class wasn't provided in FileCache constructor.

Ok, I was wrong. Honestly, I just didn't like us having another top-level dependency when one wasn't really necessary. I am aware this doesn't remove lockfile from dep tree.

@neersighted
Copy link
Member

I think this is good. I need to finish my draft of the writeup for the issue that caused 1.3.1 -- but in short, we can't reliably depend on cachecontrol to pull in lockfile, but it's silly just to pull it in to inherit from the class, when it's so minimal.

In short: not every index supports yanked packages and I'd like to see this in the codebase so we are indifferent to the presence of lockfile.

@Secrus Secrus marked this pull request as ready for review January 6, 2023 18:44
@dimbleby
Copy link
Contributor

dimbleby commented Jan 6, 2023

To my mind, simply removing a top-level dependency isn't achieving anything very useful..

And given that for the moment lockfile is an unavoidable indirect dependency, it seems to me more reasonable to use it than to duplicate it.

Still I suppose it's not worth fighting over a small amount of noise.

Edit: not entirely sure what your comment about yanked packages relates to, but if it is to do with the yanking of cachecontrol 0.12.12: suggest that a cleaner fix to make that secure would be for poetry to depend on 0.12.11 explicitly (or explicitly exclude 0.12.12, whatever)

@dimbleby
Copy link
Contributor

dimbleby commented Jan 7, 2023

On reflection I think I ought to be firmer in saying what I think about this: which is that it seems pointless and that I am against it.

Of course y'all are welcome to do what you think is right regardless: but it seems silly to have an opinion and not to say so.

@Secrus
Copy link
Member Author

Secrus commented May 24, 2023

Superseded by #7997

@Secrus Secrus closed this May 24, 2023
@Secrus Secrus deleted the drop-lockfile branch May 24, 2023 15:11
Copy link

github-actions bot commented Mar 3, 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 Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants