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

refactor the search for direct-origin dependencies #5904

Merged

Conversation

dimbleby
Copy link
Contributor

refactoring suggested by #5903.

  • all the search_for_xxx() methods returned a list of length one, which is a bit weird
  • the whole pattern is repeated three times, let's just do it once.

IMO it's kinda ugly that search_for_xxx(dependency) makes updates to dependency, that doesn't match my intuition for what such a method ought to do. But trying to refactor that broke more tests than I care to investigate.

In particular this rearrangement ensures that the updates that we make to the input dependency are the same regardless of whether we got there via the _deferred_cache or not.

@dimbleby dimbleby force-pushed the refactor-search-for-dependency branch from 415a9d3 to 3263f33 Compare June 23, 2022 20:38
@dimbleby dimbleby force-pushed the refactor-search-for-dependency branch from 75c2ecb to 25e0ac6 Compare June 25, 2022 17:41
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you thought about the following changes in behavior? (Not sure about the consequences or if it's relevant at all.)

package is not cloned anymore for file and directory dependencies. Thus, package.root_dir, package.files, package.develop are set according to the first dependency.

It seems like VCS dependencies were only put into the cache but never taken from it. Seems like a bug, but had the same effect as cloning. Now, package.develop is set according to the first dependency.

@dimbleby
Copy link
Contributor Author

@radoering yes I noticed those things, but couldn't think of a reason that they should matter - so chose to trust the test suite when it said that it saw no problem.

@radoering
Copy link
Member

Me neither. Should be fine.

@radoering radoering merged commit 63c86bf into python-poetry:master Jun 26, 2022
@dimbleby dimbleby deleted the refactor-search-for-dependency branch June 26, 2022 18:18
Copy link

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 Feb 29, 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