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

If X.Y-32 is used on non-Windows, just skip. #138

Closed
wants to merge 1 commit into from

Conversation

dhermes
Copy link
Collaborator

@dhermes dhermes commented Sep 19, 2018

Fixes #136.

@dhermes dhermes requested a review from theacodes September 19, 2018 20:52
@@ -122,9 +122,11 @@ def _resolved_interpreter(self):

Based heavily on tox's implementation (tox/interpreters.py).
"""
from nox.sessions import _SessionSkip
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@theacodes I don't like this. It was to avoid a circular import since nox.sessions imports nox.virtualenv. What do you think we should do?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just raise a new InterpreterNotFound error? We could use that as a more general purpose solution to #93.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Care to take the wheel here? I'm not really sure where to put it / what other corners to test it in.

@dhermes
Copy link
Collaborator Author

dhermes commented Sep 19, 2018

I expected some tests to fail that verified the contents of the logged error message (which were previously in RuntimeError().args). However, there was no testing of this content. WDYT @theacodes?

@dhermes
Copy link
Collaborator Author

dhermes commented Sep 24, 2018

For posterity, this was closed in favor of #140

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.

2 participants