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

Better explain Session.notify #398

Closed
rmorshea opened this issue Mar 9, 2021 · 7 comments · Fixed by #500
Closed

Better explain Session.notify #398

rmorshea opened this issue Mar 9, 2021 · 7 comments · Fixed by #500

Comments

@rmorshea
Copy link
Contributor

rmorshea commented Mar 9, 2021

As I've been working on #397 I've begun to feel that Session.notify is poorly named. Judging by the method's docstring it claims to add the named session to the execution queue. Given this, it would seem that Session.enqueue or a more verbose Session.add_to_queue would be a more descriptive name - anything to better imply that the given session will be run soon (I'm open to suggestions).

When I first heard there was a Session.notify method (not having read the docstring) a couple questions came to mind:

What is being notified?
Ok, we're probably notifying a Session, but what is that session being notified of?
Are we notifying a session while it's running?
Maybe we're notifying something else about the current session?

In the original PR, there's not really any justification for the name. The best I can come up with is to say that we are "notifying" the manifest that it should add the session to its queue, but I could only really discern that after having looked at the source to discover exactly what the method did.

@theacodes
Copy link
Collaborator

I feel like I raised the same concerns when @lukesneeringer created this and he gave me a really good example of where another project did this. I can't remember it now.

I'm happy to accept suggestions for an alias.

@cjolowicz
Copy link
Collaborator

I feel like I raised the same concerns when @lukesneeringer created this and he gave me a really good example of where another project did this. I can't remember it now.

Could it have been threading.Condition.notify from the standard library? It wakes up a thread that is waiting on a condition.

I seem to recall being slightly confused when first encountering session.notify. It didn't bother me recently, but I can see how a more intuitive name could help new users.

I'm happy to accept suggestions for an alias.

Here's another suggestion for the bikeshed then: session.schedule

Would that be more explicit about the fact that Nox will run the session when its turn comes?

@rmorshea
Copy link
Contributor Author

I think session.schedule is better than the options I proposed.

@rmorshea
Copy link
Contributor Author

rmorshea commented Mar 26, 2021

ping: @lukesneeringer thoughts on renaming Session.notify to Session.schedule?

@henryiii
Copy link
Collaborator

Simply having an example in the docs would really help. I don't mind the notify name, but was a little hard to gather what it did - an example would go a long way in helping, I think.

@rmorshea rmorshea changed the title Session.notify is poorly named Better explain Session.notify Oct 1, 2021
@rmorshea
Copy link
Contributor Author

rmorshea commented Oct 1, 2021

In retrospect, I don't think it's worth troubling people with renaming Session.notify - the core problem is that it's not documented very well. The additional API docs provided by #467 are useful, but having a section in the tutorial document would give it the exposure it needs because, at present, you have to dig through the API docs to find out about it.

@FollowTheProcess
Copy link
Collaborator

Hey all, I've added a section in the tutorial explaining session.notify a bit more. If no one objects or has any additions/comments I'm probably just going to merge that in and close this issue as it's been hanging around for a while?

Cheers 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants