Fix parsing of ~/.nestrc in show_help_with_pager() #931
Conversation
This fixes the code parsing for the pager command in ~/.nestrc which could lead to a TypeError exception when calling subprocess.call with pager=None if ~/.nestrc is an empty file (0 bytes). In addition, the code now opens ~/.nestrc safely using `with` and uses only one loop and one regular expression for parsing and uses the last '/page << /command' entry instead of the first it finds.
heplesser
added
T: Bug
| pager = pa | ||
| except: | ||
| print('Failed to open ~/.nestrc, using default pager') | ||
| pass | ||
|
|
There was a problem hiding this comment.
Why are you parsing the nestrc file manually? It is run by SLI upon startup, so you can just do something like
nest.sli_func("/page GetOptions")["command"]
to get the command in a much safer way. To have a default pager available in case nestrc does not define one, you can use
try:
pager = nest.sli_func("/page GetOptions")["command"]
except:
pager = 'more'
There was a problem hiding this comment.
@jougs The parsing code was introduced over a year ago in bdc87b7 and merged with #688. I reviewed then, and as now entirely overlooked the fact that we know the parser once NEST is loaded. So @physicalist, the solution proposed by @jougs is the way to go.
There was a problem hiding this comment.
@physicalist Thanks for the PR, it looks good except for a few details I commented on inline. Could you also send me a filled, signed and scanned contributor agreement by personal email?
| line): | ||
| pager = pa | ||
| except: | ||
| print('Failed to open ~/.nestrc, using default pager') |
There was a problem hiding this comment.
I would rather write "parse" than "open", since in principle an exception could occur during reading as well.
| pager = pa | ||
| except: | ||
| print('Failed to open ~/.nestrc, using default pager') | ||
| pass |
There was a problem hiding this comment.
pass is not needed here, please remove it.
| r'^\s?(?!%)\s?\/page\s?<<\s?\/command\s?\((\w*)', | ||
| line): | ||
| pager = pa | ||
| except: |
There was a problem hiding this comment.
PEP8 advises against bare except clauses. except IOError seems appropriate here.
There was a problem hiding this comment.
True, I meant to look up the most general but still appropriate exception class but forgot to do that :)
There was a problem hiding this comment.
For the above solution the exception class would simply be NESTError.
|
Superseeded by #948. |
This fixes a bug in the code parsing the pager command in ~/.nestrc which
could lead to a TypeError exception when calling subprocess.call with
pager=None if ~/.nestrc is an empty file (0 bytes).
I discovered this when @babsey and I discussed his changes in #928 and #913.
In addition, the code now opens ~/.nestrc safely using
withand usesonly one loop and regular expression for parsing and returns the last
/page << /commandentry instead of the first it finds.