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

Allow not passing "--upgrade" to session.install #172

Merged
merged 2 commits into from Mar 22, 2019

Conversation

s0undt3ch
Copy link
Contributor

I don't really know the reasoning why --upgrade was added by default to the pip call in session.install.
I would reason against it, but, since it has been the default, at least allow the user not to upgrade.

@theacodes
Copy link
Collaborator

This is a great point. I'm not sure why I ended up going with --upgrade by default. I'd actually be curious if we should just remove that altogether.

@dhermes WDYT?

@dhermes
Copy link
Collaborator

dhermes commented Mar 12, 2019

I think it should be off by default and have an upgrade=True override.

nox/sessions.py Outdated
@@ -242,8 +242,11 @@ def install(self, *args, **kwargs):

if "silent" not in kwargs:
kwargs["silent"] = True
upgrade = kwargs.pop("upgrade", True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am all for this but I'd make False the default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a backwards incompatible change, but I'm cool with if you're cool with it too.

@theacodes
Copy link
Collaborator

have an upgrade=True override.

At that point we could just pass it through like every other pip parameter, e.g.:

session.install("--upgrade", "-r", "requirements.txt")

@dhermes
Copy link
Collaborator

dhermes commented Mar 12, 2019

Ha, DERP. Good call

@theacodes
Copy link
Collaborator

Cool, sound good to you, @s0undt3ch?

@s0undt3ch
Copy link
Contributor Author

Sorry, just woke and now I read the whole list of comments.
Yeah, I'd rely on the existing way to pass flags, no keyword arguments, just pass the actually pip CLI flag.

@s0undt3ch
Copy link
Contributor Author

I'll update the PR.

@s0undt3ch
Copy link
Contributor Author

PR updated.

@dhermes
Copy link
Collaborator

dhermes commented Mar 12, 2019

Seems like some documentation needs to be updated as well: https://github.com/theacodes/nox/search?q=upgrade&unscoped_q=upgrade

@s0undt3ch
Copy link
Contributor Author

Docs updated.

I don't really know the reasoning why `--upgrade` was added by default
to the `pip` call in `session.install`.
I reason against it, and such flag should be passed explictily
@theacodes theacodes merged commit 5b1e1e3 into wntrblm:master Mar 22, 2019
@theacodes
Copy link
Collaborator

Thank you, @s0undt3ch!

@s0undt3ch s0undt3ch deleted the features/no-upgrade branch March 22, 2019 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants