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

Check for NOXSESSION environment variable #121

Merged
merged 7 commits into from Sep 14, 2018

Conversation

stsewd
Copy link
Collaborator

@stsewd stsewd commented Aug 26, 2018

nox/tasks.py Outdated
@@ -84,12 +86,17 @@ def filter_manifest(manifest, global_config):
the manifest otherwise (to be sent to the next task).

"""

nox_env = os.environ.get("NOXSESSION")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is possible for us to do this in __main__ using argparse?

https://github.com/theacodes/nox/blob/master/nox/__main__.py#L66

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't sure where to put it. A default arg should be enough I think

@stsewd
Copy link
Collaborator Author

stsewd commented Aug 29, 2018

So, looks like the env var from Travis is colliding with the one that nox uses. Tox removes some env variables and keeps others. Personally, I don't like that much. But here we will have problems in local dev too. Maybe we can make a simple fix on the conflicting test and move on I guess.

@theacodes
Copy link
Collaborator

theacodes commented Aug 29, 2018 via email

@@ -60,6 +61,8 @@ def test_global_config_constructor():


def test_main_no_args():
# Prevents any interference from outside
os.environ.pop("NOXSESSION", None)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if you are ok with this workaround

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind updating this to use monkeypatch.delenv? https://docs.pytest.org/en/latest/reference.html#_pytest.monkeypatch.MonkeyPatch.delenv and likewise with setenv for the test below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, that looks like what we need here

Copy link
Collaborator

@theacodes theacodes left a comment

Choose a reason for hiding this comment

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

sorry for taking so long to review this, I just have a minor comment on the tests. :)

@@ -60,6 +61,8 @@ def test_global_config_constructor():


def test_main_no_args():
# Prevents any interference from outside
os.environ.pop("NOXSESSION", None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind updating this to use monkeypatch.delenv? https://docs.pytest.org/en/latest/reference.html#_pytest.monkeypatch.MonkeyPatch.delenv and likewise with setenv for the test below?

@theacodes
Copy link
Collaborator

@stsewd I make the changes myself, I hope that's okay!

@stsewd
Copy link
Collaborator Author

stsewd commented Sep 14, 2018

That's ok, sorry I wasn't able to do it myself, busy week.

@theacodes
Copy link
Collaborator

Totally understand. :) I'm always happy to get these things over the finish line.

@theacodes theacodes merged commit 43f25a9 into wntrblm:master Sep 14, 2018
@stsewd stsewd deleted the check-nox-env branch September 14, 2018 20:08
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

2 participants