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

Lock requirements enhancement #972

Merged
merged 3 commits into from
Oct 25, 2017
Merged

Conversation

erinxocon
Copy link
Contributor

Addresses #942

@erinxocon erinxocon merged commit 13bb890 into master Oct 25, 2017
@@ -2004,9 +2026,10 @@ def uninstall(
@click.option('--python', default=False, nargs=1, help="Specify which version of Python virtualenv should use.")
@click.option('--verbose', is_flag=True, default=False, help="Verbose mode.")
@click.option('--requirements', '-r', is_flag=True, default=False, help="Generate output compatible with requirements.txt.")
@click.option('--dev', '-d', is_flag=True, default=False, help="Generate output compatible with requirements.txt for the development dependencies.")

Choose a reason for hiding this comment

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

@erinxocon I think with --dev it includes also normal packages? Should change this text to "Include dev packages into requirements.txt output"? Now it makes one think that only dev packages will be outputted.

Choose a reason for hiding this comment

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

Hmm, I think my assumption is wrong, as you wrote:

pipenv lock -r -d will give you just the development deps and development vcs deps

Ok, then the help text is according to this specification.

Apparently it's impossible to create such requirements.txt files now, that would include both standard and dev packages. I don't have use case for that, so I think it's ok. Someone else might want that, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current functionality separates them. --dev does only development dependencies, and -r only does anything listed under the default section! Is this not what was needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the all packages is needed I can add it at a later time!

Choose a reason for hiding this comment

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

The current functionality separates them. --dev does only development dependencies, and -r only does anything listed under the default section! Is this not what was needed?

Indeed, this change should fulfill my needs. Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case is that the wanted use case though? I'll let my fellow contributors put in their 2 cents!

Choose a reason for hiding this comment

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

So to clarify, I needed requirements.txt file that excludes dev packages. After the change you've made here, I can do that with pipenv lock -r as you've instructed. So yeah, my use is now supported.

I have no opinion if pipenv lock -r -d should include only dev packages or also dev packages. I personally have no use case for neither. I was just pointing out that there's now slight inconsistency in API, compared to install and update. So you might want to consider changing --dev behavior here accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate the input @tuukkamustonen! A very keen eye on the api discrepancies!

Copy link

@piotr-dobrogost piotr-dobrogost Oct 25, 2017

Choose a reason for hiding this comment

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

It seems --dev in pipenv lock -r --dev should be consistent with pipenv install --dev and pipenv update --dev indeed. In the future, if there's a need, additonal flag like --only-dev could be added to pipenv lock -r.

Choose a reason for hiding this comment

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

I just came here to post a bug stating that lock -r -d only outputs dev dependencies. In isolation, I don't think there's anything inherently wrong with the current behavior, but the fact that lock -d is inconsistent with install -d and update -d makes it seem like a bug. Not only that, but if I run pipenv lock -h, the help text for --dev says "Install both develop and default packages.", further adding to the confusion.

@techalchemy
Copy link
Member

techalchemy commented Oct 25, 2017

@piotr-dobrogost @erinxocon @tuukkamustonen I feel it’s not worth getting too sidetracked over this one. It is a small inconsistency and I like the —dev-only suggestion to maintain similar functionality.

Erin: stop giving the people what they want! Then they will have it and we won’t have any more things to do :)

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.

5 participants