-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
Addition of session.notify #39
Conversation
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.
Looks mostly good, just a few questions.
@@ -60,7 +60,7 @@ def __init__(self, args, env=None, silent=False, path=None, | |||
self.path = path | |||
self.success_codes = success_codes or [0] | |||
|
|||
def run(self, path_override=None, env_fallback=None): | |||
def run(self, path_override=None, env_fallback=None, **kwargs): |
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.
why add kwargs
and not use them?
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.
The invocation of Command
is now sending an additional argument, which is used by NotifyCommand.__call__
. This means I do not have to have a separate if check for every command subclass (there could eventually be more).
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.
Gotcha
@@ -164,6 +164,37 @@ def make_session(self, name, func): | |||
def next(self): | |||
return self.__next__() | |||
|
|||
def notify(self, session): |
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.
How does this work with parametrized 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.
You can specify either the name or the parameterized name:
So both of these would work:
notify('unit_tests')
notify("unit_tests(python_version='3.6')")
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.
It would be nice t be able to do notify(unit_test, python_version='3.6')
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.
It would have to be notify('unit_tests', python_version='3.6')
. I agree that would be nice but assert it can be separate. (Both forms can work.)
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.
that's fine
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.
Do you want to add this in a follow-up PR? If so, LGTM.
This adds the
session.notify
command, exemplified in nox's ownnox.py
module, where any of the four interpreters trigger thecover
session.