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

pipenv lock -r respect pypi_mirror args #4200

Merged
merged 7 commits into from Apr 25, 2020
Merged

Conversation

TennyZhuang
Copy link
Contributor

Thank you for contributing to Pipenv!

The issue

What is the thing you want to fix? Is it associated with an issue on GitHub? Please mention it.

#4199

The fix

How does this pull request fix your problem? Did you consider any alternatives? Why is this the best solution, in your opinion?

extend the pip_args in the first line of requirements.txt.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

@techalchemy techalchemy added Category: Dependency Resolution Issue relates to dependency resolution. Category: Private PyPIs 😎 Problem relates to private PyPI usage. good first issue Issues suitable as a newcomer to get familiar with Pipenv! Priority: Low This item is low priority and may not be looked at in the next few release cycles. Type: Enhancement 💡 This is a feature or enhancement request. labels Apr 23, 2020
Copy link
Member

@techalchemy techalchemy left a comment

Choose a reason for hiding this comment

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

Thanks for the pr! I've requested a small change to avoid triggering exceptions for users with multiple indexes declared in their Pipfile, and if you would, please add a news entry (e.g. news/4199.bugfix.rst) with a sentence describing the fix included in the PR, e.g.:

When writing pip-compatible requirements using pipenv lock -r while passing in a --pypi-mirror, the mirror url will now be included instead of the url in the Pipfile.

@@ -791,7 +791,7 @@ def do_install_dependencies(
skip_lock=False,
concurrent=True,
requirements_dir=None,
pypi_mirror=False,
pypi_mirror=None,
Copy link
Member

Choose a reason for hiding this comment

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

This probably shouldn't need to change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why the pypi_mirror becoms False default here but None in the rest parts of codes?

Copy link
Member

Choose a reason for hiding this comment

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

Most likely there is no good reason for this -- and apologies for not taking the time to look at it more closely! If, as you say, this is None everywhere else, and we don't differentiate in the function below (e.g. by checking if it's False) then it will be ok to make this change for consistency

pipenv/core.py Outdated Show resolved Hide resolved
@techalchemy techalchemy added PR: awaiting-news The PR related to this issue is missing a news fragment and cannot be merged. Status: Changes Requested This item is currently pending changes requested from the author. labels Apr 23, 2020
Copy link
Member

@techalchemy techalchemy left a comment

Choose a reason for hiding this comment

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

Thanks for the quick update! looks good now

@@ -791,7 +791,7 @@ def do_install_dependencies(
skip_lock=False,
concurrent=True,
requirements_dir=None,
pypi_mirror=False,
pypi_mirror=None,
Copy link
Member

Choose a reason for hiding this comment

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

Most likely there is no good reason for this -- and apologies for not taking the time to look at it more closely! If, as you say, this is None everywhere else, and we don't differentiate in the function below (e.g. by checking if it's False) then it will be ok to make this change for consistency

@techalchemy techalchemy removed the PR: awaiting-news The PR related to this issue is missing a news fragment and cannot be merged. label Apr 25, 2020
@techalchemy techalchemy merged commit f470ec7 into pypa:master Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Dependency Resolution Issue relates to dependency resolution. Category: Private PyPIs 😎 Problem relates to private PyPI usage. good first issue Issues suitable as a newcomer to get familiar with Pipenv! Priority: Low This item is low priority and may not be looked at in the next few release cycles. Status: Changes Requested This item is currently pending changes requested from the author. Type: Enhancement 💡 This is a feature or enhancement request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants