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

Add mechamism for explicit requires package repository references #2057

Closed

Conversation

sarayourfriend
Copy link
Contributor

Part of #1270.

Does not close out #1270 because @freakboy3742 also requested adding support for pip's --find-links. I chose to leave that out of this PR because this PR already has enough edge cases to consider (see the issue for discussion about them), and supporting --find-links requires thinking about how to generically describe it.

While this PR is feature complete for --index-url and --extra-index-url, which were the original concerns of the linked issue, I'm leaving it as a draft in order to propose a very different approach, which might simplify description of the feature, and better anticipate future changes like support/hooks for different package installation backends.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

with self.input.wait_bar("Writing requirements file..."):
with requirements_path.open("w", encoding="utf-8") as f:
if requires:
if requires or extra_pip_args:
Copy link
Member

Choose a reason for hiding this comment

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

My inclination is that this is the right idea, not taken far enough. If we're generating a new file, we should always be writing the timestamp - worst case, we end up with a file that only contains the timestamp, rather than a literally empty file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally, makes sense to do away with the check altogether.

Also noting that I'd originally moved the check to the function root and done an early return when nothing would be written, but if requirements are being removed, it still needs to write the file to reflect requirements changed. Removing the conditional on the timestamp write makes that aspect a bit clearer as well (maybe with a small comment to help explain).

@sarayourfriend
Copy link
Contributor Author

Closing this based on the discussion in the linked issue, and in particular this comment from Russell confirming the other approach.

I'll work on a changeset for that this week and hope to have a PR up in the next couple of days 🙂

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