-
Notifications
You must be signed in to change notification settings - Fork 22
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
Golem.start() and Golem.stop() #644
Conversation
…on-context-manager
0043832
to
f489319
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.
Looks good, left some minor naming/docstring comments.
@@ -113,19 +115,41 @@ def subnet_tag(self) -> Optional[str]: | |||
"""Return the name of the subnet, or `None` if it is not set.""" | |||
return self._engine.subnet_tag | |||
|
|||
async def __aenter__(self) -> "Golem": | |||
@property | |||
def operative(self) -> bool: |
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 name of this property is somewhat vague, at least to me. How about we rename this to just started
?
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.
This property is essentially "There was a start
that had no stop
after it".
I'm not sure if this is "started" - if you start and that stop something it might be both started
and stopped
at the same time (and that might be different than not having started at all).
Maybe "active" would be better? Or "running"?
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'm fine with either... honestly
async def __aenter__(self) -> "Golem": | ||
@property | ||
def operative(self) -> bool: | ||
"""Return True if Golem started and didn't stop""" |
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.
"""Return True if Golem started and didn't stop""" | |
"""Return `True` if this instance is currently started, `False` otherwise.""" |
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.
As in the previous comment, I'm not sure if "started" is a good name.
btw, I wonder - do we think it's sensible to leave two examples out there? as in, leave the issue I have with updating @zakaprov @johny-b ? |
Co-authored-by: Kuba Mazurek <[email protected]>
I don't like this. I think we already have too many examples that differ only on a few lines and have dozens copy-pasted lines.
That's a good point. |
@shadeofblue I think you should make the decision : ) |
the only issue I have with modifying some other example is that maybe somebody will think that e.g. to interact with a VPN you have to use non-context-manager Golem... that's why having two, otherwise identical, examples side-by-side would, I think be clearer for the outside world |
resolves #561
resolves #606