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.

Sign up for GitHub

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

Lock requirements enhancement #972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
105a2fc
Added dev option to lock -r to generate requirements with and without…
erinxocon Oct 25, 2017
5b7d231
Add ability to export default and dev requirements separately
erinxocon Oct 25, 2017
e5da54b
updated test for new lock -r functionality
erinxocon Oct 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions pipenv/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,11 +767,21 @@ def cleanup_procs(procs, concurrent):
deps = {}
vcs_deps = {}

# Store dev only deps for a requirements output
dev_deps = {}
dev_vcs_deps = {}

# Add development deps if --dev was passed.
if dev:
deps.update(lockfile['develop'])
vcs_deps.update(lockfile.get('develop-vcs', {}))

# Add only dev deps if requirements was passed
if requirements:
dev_deps.update(lockfile['develop'])
dev_vcs_deps.update(lockfile.get('develop-vcs', {}))


# Install default dependencies, always.
deps.update(lockfile['default'] if not only else {})
vcs_deps.update(lockfile.get('default-vcs', {}))
Expand All @@ -791,8 +801,20 @@ def cleanup_procs(procs, concurrent):

# --requirements was passed.
if requirements:
click.echo('\n'.join(d[0] for d in deps_list))
sys.exit(0)
# Output only default dependencies
if not dev:
click.echo('\n'.join(d[0] for d in deps_list))
sys.exit(0)

# Output only dev dependencies
if dev:
dev_deps_list = [(d, ignore_hashes, blocking) for d in convert_deps_to_pip(dev_deps, project, r=False, include_index=True)]
if len(dev_vcs_deps):
dev_deps_list.extend((d, True, True) for d in convert_deps_to_pip(dev_vcs_deps, project, r=False))

click.echo('\n'.join(d[0] for d in dev_deps_list))
sys.exit(0)


procs = []

Expand Down Expand Up @@ -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.")
Copy link

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.

Copy link

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!

Copy link

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!

Copy link

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 •
edited
Loading

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.

Copy link

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.

@click.option('--clear', is_flag=True, default=False, help="Clear the dependency cache.")
@click.option('--pre', is_flag=True, default=False, help=u"Allow pre–releases.")
def lock(three=None, python=False, verbose=False, requirements=False, clear=False, pre=False):
def lock(three=None, python=False, verbose=False, requirements=False, dev=False, clear=False, pre=False):

# Ensure that virtualenv is available.
ensure_project(three=three, python=python)
Expand All @@ -2016,7 +2039,7 @@ def lock(three=None, python=False, verbose=False, requirements=False, clear=Fals
pre = project.settings.get('pre')

if requirements:
do_init(dev=True, requirements=requirements)
do_init(dev=dev, requirements=requirements)

do_lock(verbose=verbose, clear=clear, pre=pre)

Expand Down
12 changes: 10 additions & 2 deletions tests/test_pipenv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ def test_shell_nested_venv_in_project(self):
# Check that .venv now shows in pew's managed list
pew_list = delegator.run('pew ls')
assert '.venv' in pew_list.out
# Check for the venv directory
# Check for the venv directory
c = delegator.run('pew dir .venv')
# Compare pew's virtualenv path to what we expect
venv_path = get_windows_path(project.project_directory, '.venv')
Expand Down Expand Up @@ -668,13 +668,21 @@ def test_lock_requirements_file(self):
""".strip()
f.write(contents)

req_list = ("requests==2.14.0", "flask==0.12.2", "pytest==3.1.1")
req_list = ("requests==2.14.0", "flask==0.12.2")

dev_req_list = ("pytest==3.1.1")

c = p.pipenv('lock -r')
d = p.pipenv('lock -r -d')
assert c.return_code == 0
assert d.return_code == 0

for req in req_list:
assert req in c.out

for req in dev_req_list:
assert req in d.out

@pytest.mark.lock
@pytest.mark.complex
def test_complex_lock_with_vcs_deps(self):
Expand Down