Miscellaneous minor tune ups, shouldn't effect functioning #100
Conversation
…ency with spacing
…atch_script first
…on call signature For consistency with jupyterhub itself
batchspawner/batchspawner.py
Outdated
| if self.job_status and re.search(self.state_running_re, self.job_status): | ||
| return True | ||
| else: return False | ||
| return bool(self.job_status and re.search(self.state_running_re, self.job_status)) |
There was a problem hiding this comment.
these two should be the only possibly "functional" changes
|
Looks good to me, though I'm not a maintainer of this. The travis failure seems to be a random timeout issue. |
|
Thanks @rkdarst ... unfortunately I do not have super powers to restart that job to confirm that it was something intermittent, so would need the maintainer(s) to restart it |
|
Re-running now |
There was a problem hiding this comment.
@yarikoptic Thanks for the PR. A few corrections to the PR and then it should be ready to approve. Thanks.
batchspawner/batchspawner.py
Outdated
| @@ -334,7 +335,7 @@ def start(self): | |||
| if self.user and self.user.server and self.user.server.port: | |||
| self.port = self.user.server.port | |||
| self.db.commit() | |||
| elif (jupyterhub.version_info < (0,7) and not self.user.server.port) or \ | |||
| elif (jupyterhub.version_info < (0,7) and not self.user.server.port) or \ | |||
There was a problem hiding this comment.
Hi @yarikoptic, In general, we discourage PRs that are primarily style changes. In this case, I'm happy to be rid of the \. You may as well go ahead and eliminate the \ here by placing outer parens on both conditionals.
There was a problem hiding this comment.
Alternatively:
elif (jupyterhub.version_info < (0,7) and not self.user.server.port) or (
jupyterhub.version_info >= (0,7) and not self.port
):
batchspawner/batchspawner.py
Outdated
| if self.job_status and re.search(self.state_pending_re, self.job_status): | ||
| return True | ||
| else: return False | ||
| return bool(self.job_status and re.search(self.state_pending_re, self.job_status)) |
There was a problem hiding this comment.
I recommend leaving as currently written but clean up the else statement to put the return on a separate line.
By adding the bool(), Python will not short-circuit the expression if self.job_status evaluates as false and will do the regex search even if not needed.
There was a problem hiding this comment.
Are you sure @willingc about the differently behaving "short-circuit" between if and bool? I am not aware of any difference in this case since in both cases it needs first to evaluate the conditional for its bool value, and thus it would short-circuit (stop evaluating as soon as the first one evaluates to False) in either of the cases... Since I trust no-one, including myself, here is a quick demo to support that:
$> cat /tmp/shortc.py
def func(x):
print("in func")
return bool(x)
import sys
b = bool(int(sys.argv[1]))
v = int(sys.argv[2])
print("b=%r v=%r" % (b,v))
if b and func(v):
print("if True")
else:
print("if False")
print("bool %s" % bool(b and func(v)))
$> python3.7 /tmp/shortc.py 0 1
b=False v=1
if False
bool False
$> python3.7 /tmp/shortc.py 1 1
b=True v=1
in func
if True
in func
bool True
$> python3.7 /tmp/shortc.py 1 0
b=True v=0
in func
if False
in func
bool False
$> python3.7 /tmp/shortc.py 0 0
b=False v=0
if False
bool FalseThe only advantage I see if of the original way (and I will change back now, since I do not have strong opinion on this) is that you could easier monitor the coverage -- which of the cases was triggered by the tests code, and which possibly not, which isn't possible if it is just a one line expression
|
done, pushed tuneups, Cheers! |
I was just looking at the code so introduced some minor changes for consistency (within, with PEP-8, and with jupyterhub). Feel free to ignore/discard.