-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
feat: support setting an empty session list #523
Conversation
32eba77
to
02ede6f
Compare
@henryiii LGTM, just thinking maybe there should be a quick mention in the tutorial section of the docs? Probably fits best under the |
See #465 (comment) |
2191d94
to
24c48a0
Compare
8254716
to
9f5a380
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this, this is looking great!
Here are a few minor things I noticed. I haven't had time to review the changed control flow yet.
Looking good overall! My only (minor) nit is that if you set However if you run I have two thoughts:
However this could just be my OCD showing so I'm happy to be overruled if we're happy with this as is 🙂 |
nox/tasks.py
Outdated
if ( | ||
not global_config.keywords | ||
and not global_config.pythons | ||
and not global_config.sessions | ||
and not global_config.list_sessions | ||
and global_config.sessions is not None | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the intent could be expressed a little more clearly here. How do you feel about the suggestion below?
I omitted the checks for keywords and pythons because these conditions would have led to bailing out earlier.
if ( | |
not global_config.keywords | |
and not global_config.pythons | |
and not global_config.sessions | |
and not global_config.list_sessions | |
and global_config.sessions is not None | |
): | |
# List sessions if `nox.options.sessions` is an empty sequence. | |
if ( | |
global_config.sessions is not None | |
and not global_config.sessions | |
and not global_config.list_sessions | |
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I redid this, but I kept the keyword/python filtering, since nox -k keyword
should select and run the tests with keyword
, rather than forcing it to also be passed via -s
, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #523 (comment)
I agree with @FollowTheProcess that we don't need to special-case the listing for the "no default sessions" case. To be precise, the sessions can be displayed with a minus because they are all deselected, and the explanatory sentence at the end can be kept. (Codewise, this would amount to dropping the ˋselectˋ parameter in ˋproduce_listingˋ.) |
Also I'm confused, why are there asterisks in the screenshot if all sessions are deselected? |
e2d0424
to
947b352
Compare
I realize now that you don't filter sessions by name when Can we change the condition for this filter to check for You will need to check for a non-empty manifest after filtering by session name, like for the other filters. If the check fails, you can invoke I believe there are three good reasons for this change:
As for selecting sessions by keyword even when What do you think? Update: My suggestion breaks some tests in |
nox/tasks.py
Outdated
and not global_config.pythons | ||
): | ||
manifest.filter_by_name(global_config.sessions) | ||
print("No sessions selected. Please select a session with -s <session name>.\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the logger here? It would make the message stand out from the session listing.
print("No sessions selected. Please select a session with -s <session name>.\n") | |
logger.warning( | |
"No sessions selected. Please select a session with -s <session name>.\n" | |
) |
We'd need to adapt the new tests to use caplog.text
instead of capsys.readouterr().out
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it an error / warning though? The session listing is a print.
Co-authored-by: Claudio Jolowicz <cjolowicz@gmail.com>
Co-authored-by: Tom Fleet <tomfleet2018@gmail.com>
Ah, this was the piece I didn't know. I wanted to put the check here, but didn't realize it would not cause an issue with filtering. It's at all not obvious that this will be ignored from the logic in this function! Seems like it should have it in the if statement. |
947b352
to
b880bbf
Compare
(Haven't tested the recent comment by hand with the noxfile yet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much @henryiii for all the work that went into this
Closes #465.
Setting an empty session list in the noxfile will default to listing the available sessions.