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

Issue #3316: Include all deps in 'pipenv lock -r --dev' #4183

Merged
merged 12 commits into from
May 20, 2020

Conversation

ncoghlan
Copy link
Member

@ncoghlan ncoghlan commented Apr 10, 2020

  • Implements PEEP-006
  • Closes [Cli Docs?] Conflicting Info in regards to pipenv lock --dev --requirements #3316
  • pipenv lock -r --dev is now consistent with other commands
    and the CLI help output, and includes both default and dev
    dependencies in the result
  • New --dev-only option allows requesting the previous behaviour
    (which was specifically designed to support the traditional
    requirements.txt/dev-requirements.txt split)
  • Adjusts several requirements variables to use either requirementstxt or
    emit_requirements to make it clearer whether they're storing a reference to an
    actual requirements file, or a flag that says to emit the requirements information
    without actually installing anything.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .feature

* Implements PEEP-006
* `pipenv lock -r --dev` is now consistent with other commands
  and the CLI help output, and includes both default and dev
  dependencies in the result
* New `--dev-only` option allows requesting the previous behaviour
  (which was specifically designed to support the traditional
  `requirements.txt`/`dev-requirements.txt` split)
pipenv/core.py Outdated
# Load the lockfile if it exists, or if only is being used (e.g. lock is being used).
if skip_lock or only or not project.lockfile_exists:
# Load the lockfile if it exists, or if dev_only is being used.
if skip_lock or dev_only or not project.lockfile_exists:
Copy link
Member Author

@ncoghlan ncoghlan Apr 10, 2020

Choose a reason for hiding this comment

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

@techalchemy @frostming I'm not sure why dev_only (formerly called only) was ever in this conditional. To get PEEP 6 to work as intended, I think I may actually need to remove it, and follow the same branch as the regular non-dev requirements.

Copy link
Member

Choose a reason for hiding this comment

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

Looking over the commit history for this, I have never touched this line which makes sense as I have no idea what this argument does. This is very confusing. If it is meant to be dev_only it wouldn't make any sense to skip locking...

Upon investigation, do_install_dependencies is only invoked by one caller and that caller does not pass this argument.

@ncoghlan
Copy link
Member Author

@techalchemy @frostming I don't think this is urgent (it doesn't need to go in the next release), making an initial attempt at implementing it just happened to make it to the top of my todo list today.

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.

This looks good with a few incredibly minor tweaks

pipenv/cli/options.py Show resolved Hide resolved
pipenv/core.py Outdated
# Load the lockfile if it exists, or if only is being used (e.g. lock is being used).
if skip_lock or only or not project.lockfile_exists:
# Load the lockfile if it exists, or if dev_only is being used.
if skip_lock or dev_only or not project.lockfile_exists:
Copy link
Member

Choose a reason for hiding this comment

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

Looking over the commit history for this, I have never touched this line which makes sense as I have no idea what this argument does. This is very confusing. If it is meant to be dev_only it wouldn't make any sense to skip locking...

Upon investigation, do_install_dependencies is only invoked by one caller and that caller does not pass this argument.

pipenv/core.py Outdated Show resolved Hide resolved
news/3316.feature.rst Outdated Show resolved Hide resolved
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, this looks good to me now!

@techalchemy techalchemy merged commit db74ee6 into pypa:master May 20, 2020
@ncoghlan
Copy link
Member Author

Thanks for finishing it off!

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.

[Cli Docs?] Conflicting Info in regards to pipenv lock --dev --requirements
2 participants