Details
- Reviewers
spectral - Group Reviewers
hg-reviewers - Commits
- rHG8f54d9c79b12: dirstate: add missing return on platforms without exec or symlink
Diff Detail
- Repository
- rHG Mercurial
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
I'm not sure the etiquette here; I'm accepting the revision since it obviously fixes a bug, but still would like the comments to be considered/responded to?
| mercurial/dirstate.py | ||
|---|---|---|
| 303 | I think it'd be easier to read and easier to see the issue if this used fewer returns: if self._checklink:
def f(x):
...
elif self._checkexec:
def f(x):
...
else:
def f(x):
...
return fIdeally we'd be able to get the first branch as well, perhaps like: def flagfunc(self, buildfallback):
# fallback is unused in this case and expensive to calculate
if not (self._checklink and self._checkexec):
fallback = buildfallback()
if self._checklink and self._checkexec:
def f(x):
...
elif self._checklink:
...
return fI would also accept moving these functions to be at module scope with "obvious" names like flagfunc_link_and_exec, flagfunc_link_only, flagfunc_exec_only, flagfunc_fallback_only (or whatever), and shrinking flagfunc considerably by just making it return the appropriate function, which would make it small enough to make the forgotten return quite a bit more obvious. I'm normally a fan of early returns (I don't think there's a strong reason for functions to have "only one return"), but I think in this case it would improve readability and maintainability. | |
I think it's fine to accept a change to say "I'm ok with it going through like so because it's important" and expect at the same time to get an answer for the other comments. I've done it multiple times with the author then updating the patch or sending a follow-up.
| mercurial/dirstate.py | ||
|---|---|---|
| 303 | I agree that factoring the returns is a good idea here. I'm not so sure that moving the functions to module scope is worth the (albeit very small churn) since they are so small and don't offer much use outside of this context. | |
| mercurial/dirstate.py | ||
|---|---|---|
| 303 | Yeah, keeping them inlined but with useful names and having the decision tree be short would also be fine, not sure why I didn't think of that before "move them to module scope" :) Something like: def flagfunc(self, buildfallback):
if not (self._checklink and self._checkexec):
fallback = buildfallback()
def check_both(x):
...
def check_link(x):
...
if self._checklink and self._checkexec:
return check_both
elif self._checklink:
return check_link
... | |
I think it'd be easier to read and easier to see the issue if this used fewer returns:
if self._checklink: def f(x): ... elif self._checkexec: def f(x): ... else: def f(x): ... return fIdeally we'd be able to get the first branch as well, perhaps like:
def flagfunc(self, buildfallback): # fallback is unused in this case and expensive to calculate if not (self._checklink and self._checkexec): fallback = buildfallback() if self._checklink and self._checkexec: def f(x): ... elif self._checklink: ... return fI would also accept moving these functions to be at module scope with "obvious" names like flagfunc_link_and_exec, flagfunc_link_only, flagfunc_exec_only, flagfunc_fallback_only (or whatever), and shrinking flagfunc considerably by just making it return the appropriate function, which would make it small enough to make the forgotten return quite a bit more obvious.
I'm normally a fan of early returns (I don't think there's a strong reason for functions to have "only one return"), but I think in this case it would improve readability and maintainability.