394 integrates no install flag #411

gwynforthewyn · 2021-03-27T23:38:13Z

Adds a new option, --no-install. This is used to skip installing packages if the packages have been installed once.
Adds a boolean to the virtualenv base class, venv_created, which tracks whether or not the virtual environment has already been created. The --no-install flag behaves differently dependent upon whether a venv is already created or not.
- If no virtualenv is present, a message is logged that no-install is being ignored and normal installations proceed.
- If a virtualenv is already present, --no-install prevents the installation of packages.
Adds tests to validate the above
Also refactors SessionNoSlots into an extracted class MockableSession. This followed from scratching my head trying to understand what SessionNoSlots provided, and then heading to the PR which implemented it (Adding Session.__slots__. #128) to see what the class was buying the tests. I'm happy to put this back how I found it 😅

Closes #394

cjolowicz

Hi @jamandbees thanks for the new PR 👍

Can you please run nox -s blacken in your branch? This should take care of the failed lint session in CI.

I left a few more quick comments inline.

nox/sessions.py

nox/virtualenv.py

gwynforthewyn · 2021-03-29T16:33:18Z

Hi @jamandbees thanks for the new PR 👍

Can you please run nox -s blacken in your branch? This should take care of the failed lint session in CI.

I left a few more quick comments inline.

I didn't make this clear, but linting etc are all passing now. 🙂

noxfile.py

nox/sessions.py

cjolowicz

Thanks, looking nice! Some more comments inline.

nox/sessions.py

cjolowicz

Hey @jamandbees thanks for the update! Here are some suggestions. This should also take care of the coverage woes you mentioned.

docs/config.rst

nox/sessions.py

cjolowicz

Hey @jamandbees here's a fresh review, see the inline comments 👍

I think we're quite close to wrapping this up. The shorthand for -r --no-install and the documentation can also be left to a follow-up PR. But feel free to have a go at these if you want to.

In case you do want to tackle the shorthand option: I liked the option name you proposed (the second one). Also, a short option -R would make sense to me (mirroring -r for plain reuse). You could probably implement this using a finalizer function, see _color_finalizer in _options.py for an example.

cjolowicz · 2021-04-29T20:29:11Z

nox/virtualenv.py

@@ -399,6 +402,8 @@ def create(self) -> bool:
                    self.location_name
                )
            )
+            self._reused = False


This should be True not False, because the virtualenv is being reused.

Suggested change

self._reused = False

self._reused = True

(BTW it would be great if the tests would catch this.)

I've added a couple of assertions for _reused, one in the tests/test_virtualenv.py in test_condaenv_create and the other in test_create.

I think we're quite close to wrapping this up. The shorthand for -r --no-install and the documentation can also be left to a follow-up PR. But feel free to have a go at these if you want to.

@cjolowicz I think it makes sense to relegate those to a follow-up PR. I think I've had this one hanging out for long enough 🙂

Thanks for your help and patience so far!

nox/sessions.py

…ntrblm#394) * skips pip installs * skips conda installs * skips run_always if no-install is set

…trblm#394) reuse_existing is now true if we are not installing pacakges.

This boolean informs us whether we need to ignore the "no-install" command line option.

Extracted and renamed SessionNoSlots to MockableSession to clarify what it is used for. Reduced duplication of that code. Duplicated several tests for conda installs but ensured that the no-install flag was set and ignored.

* Updates venv_created to a private data member. * Removes irrelevant interpolation from no-install log messages.

Also removed unnecessary whitespace lines. Co-authored-by: Claudio Jolowicz <cjolowicz@gmail.com>

@cjolowicz

This was good advice from @cjolowicz helping to clarify the intention of this boolean.

_clean_location already detects whether a venv is being reused or not, so setting the boolean within this is safe. whether a venv exists or not.

…#394) Also reduces log verbosity by only noting package reinstallation when venv is not explicitly reused.

Removes unneeded logger messages when --no-install is ignored. Simplifies tests and restores 100% test coverage.

Also removes logging for no_install.

cjolowicz · 2021-05-23T09:02:58Z

@jamandbees Thanks for the update! I enabled your CI run, and it looks like some tests need adapting to your latest changes. Could you take a look?

gwynforthewyn · 2021-05-25T19:45:56Z

@cjolowicz I'm honestly not sure how to adapt them; when I check git, I get the impression that this is from a recent merge, but I'm not confident that I understand what's going on.

cjolowicz · 2021-05-28T14:27:28Z

Hey @jamandbees , no problem and thanks for your work so far! I picked this up in #432 and credited you as co-author.

cjolowicz reviewed Mar 27, 2021

View reviewed changes

nox/sessions.py Outdated Show resolved Hide resolved

nox/sessions.py Outdated Show resolved Hide resolved

nox/sessions.py Outdated Show resolved Hide resolved

nox/virtualenv.py Outdated Show resolved Hide resolved

gwynforthewyn force-pushed the 394-integrates-no-install-flag branch 2 times, most recently from 394f9c4 to 8262d67 Compare March 28, 2021 00:05

cjolowicz mentioned this pull request Mar 29, 2021

Add an option to skip install commands #394

Closed

cjolowicz reviewed Mar 29, 2021

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

cjolowicz reviewed Mar 29, 2021

View reviewed changes

nox/sessions.py Outdated Show resolved Hide resolved

cjolowicz reviewed Mar 29, 2021

View reviewed changes

nox/sessions.py Outdated Show resolved Hide resolved

cjolowicz reviewed Mar 29, 2021

View reviewed changes

nox/sessions.py Outdated Show resolved Hide resolved

gwynforthewyn force-pushed the 394-integrates-no-install-flag branch 2 times, most recently from 4ac9bc1 to 7144b11 Compare April 3, 2021 15:28

cjolowicz reviewed Apr 18, 2021

View reviewed changes

docs/config.rst Outdated Show resolved Hide resolved

nox/sessions.py Outdated Show resolved Hide resolved

nox/sessions.py Outdated Show resolved Hide resolved

cjolowicz reviewed Apr 29, 2021

View reviewed changes

gwynforthewyn and others added 12 commits May 6, 2021 19:30

Adds option to skip Session dependency installs. Default is False. (w…

b529d7f

…ntrblm#394) * skips pip installs * skips conda installs * skips run_always if no-install is set

Adds check for no_install flag to check for reusing existing venv (wn…

6969cda

…trblm#394) reuse_existing is now true if we are not installing pacakges.

Adds tracking of whether venv has been create (wntrblm#394)

1be3a72

This boolean informs us whether we need to ignore the "no-install" command line option.

Adds no-install flag to condaenv venv (wntrblm#394)

6639daf

Extracted and renamed SessionNoSlots to MockableSession to clarify what it is used for. Reduced duplication of that code. Duplicated several tests for conda installs but ensured that the no-install flag was set and ignored.

Supports no-install flag in condaenv type virtualenv (wntrblm#394)

7ac3fab

Linting (wntrblm#394)

0d224b0

PR feedback from @cjolowicz

ce3c1e6

* Updates venv_created to a private data member. * Removes irrelevant interpolation from no-install log messages.

Inadvertently committed a removal of the '--check' flag for the linter.

2f38d98

Also removed unnecessary whitespace lines. Co-authored-by: Claudio Jolowicz <cjolowicz@gmail.com>

Rename refactoring venv_created -> reused. (wntrblm#394)

4120470

This was good advice from @cjolowicz helping to clarify the intention of this boolean.

Move _reused behind ._clean_location (wntrblm#394)

36a1e41

_clean_location already detects whether a venv is being reused or not, so setting the boolean within this is safe. whether a venv exists or not.

Removes automatic reuse of venvs when no-install is provided (wntrblm…

31529de

…#394) Also reduces log verbosity by only noting package reinstallation when venv is not explicitly reused.

PR review cleanups (wntrblm#394)

6ac8898

Removes unneeded logger messages when --no-install is ignored. Simplifies tests and restores 100% test coverage.

gwynforthewyn force-pushed the 394-integrates-no-install-flag branch 2 times, most recently from 5498014 to 61dbefe Compare May 8, 2021 16:40

Adds tests for _reused wntrblm#394

0a088ae

Also removes logging for no_install.

gwynforthewyn force-pushed the 394-integrates-no-install-flag branch from 61dbefe to 0a088ae Compare May 8, 2021 16:41

gwynforthewyn closed this May 25, 2021

cjolowicz mentioned this pull request May 28, 2021

Add option --no-install to skip install commands in reused environments #432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

394 integrates no install flag #411

394 integrates no install flag #411

gwynforthewyn commented Mar 27, 2021 •

edited by cjolowicz

cjolowicz left a comment

gwynforthewyn commented Mar 29, 2021

cjolowicz left a comment

cjolowicz left a comment

cjolowicz left a comment

cjolowicz Apr 29, 2021

cjolowicz Apr 29, 2021

gwynforthewyn May 8, 2021

gwynforthewyn May 8, 2021 •

edited

cjolowicz commented May 23, 2021

gwynforthewyn commented May 25, 2021

cjolowicz commented May 28, 2021

394 integrates no install flag #411

394 integrates no install flag #411

Conversation

gwynforthewyn commented Mar 27, 2021 • edited by cjolowicz

cjolowicz left a comment

Choose a reason for hiding this comment

gwynforthewyn commented Mar 29, 2021

cjolowicz left a comment

Choose a reason for hiding this comment

cjolowicz left a comment

Choose a reason for hiding this comment

cjolowicz left a comment

Choose a reason for hiding this comment

cjolowicz Apr 29, 2021

Choose a reason for hiding this comment

cjolowicz Apr 29, 2021

Choose a reason for hiding this comment

gwynforthewyn May 8, 2021

Choose a reason for hiding this comment

gwynforthewyn May 8, 2021 • edited

Choose a reason for hiding this comment

cjolowicz commented May 23, 2021

gwynforthewyn commented May 25, 2021

cjolowicz commented May 28, 2021

gwynforthewyn commented Mar 27, 2021 •

edited by cjolowicz

gwynforthewyn May 8, 2021 •

edited