Review: Approve
Thank you! Small comments and suggestions follow.
I asked about reordering the functions for alphabetization. Not having run at
the top is somewhat surprising to me, but I am ok with it.
I was struck that file_append really expects the line to be added to end with a
\n. I suppose we could enforce that one way or another...or not. I'm OK with
leaving as is, or you could add a \n if the line does not already end with one
(and do this before you check if the line is in the content). On a somewhat
related note, it might be better to check if the line to be added is in the
output of readlines(), not read(); that would verify that the line, not a line
that ends with the line, is in the file. For example, right now a file of
"abc\ndef\n" would incorrectly show that the "ef\n" line was in the file.
In file_prepend, I am tempted to suggest that this snippet should be changed:
if line in lines:
lines.remove(line)
This iterates through the lines twice, which is mildly annoying to me but
almost certainly not a practical concern for us with these use cases. That
said, I'd be tempted to change that to the following:
try:
lines.remove(line)
except ValueError:
pass
You could also verify that the line to be added in this function ended with a
\n if you wanted to.
I don't really understand this part of the docstring for get_su_command:
This can be used together with `run` when the `su` context manager is not
enough (e.g. an external program uses uid rather than euid).
Oh! You mean that you would use run(*get_su_command(...)). Maybe an example
would help.
I'm a bit concerned about what you are doing here in get_value_from_line:
return line.split('=')[1].strip('"\' ')
would maybe json be a safer approach? No, I tried it, and json.loads('"hi"')
is fine but json.loads("'hi'") is not. :-/ I dunno. When do we use this? The
logic seems a bit idiosyncratic.
The grep command also seems a bit idiosyncratic. Why is it reasonable to
always strip the result? I also would be tempted to rename the funtion: grep
does a lot more than that. simply search_file? Hm, I see that you just
reordered this file, you didn't actually add grep.... Never mind then, I
guess. I should have commented on this sooner, but you shouldn't have to worry
about it. If you *do* want to change it, please leave the "grep" alias around
for now so that our charms do not suddenly break.
I don't understand why you removed the Serializer. Was it defined twice?
I thought the choices you made for doc examples and for unit tests were good.
Thank you.
Great job!
Gary
--
https://code.launchpad.net/~frankban/python-shell-toolbox/helpers/+merge/96180
Your team Launchpad Yellow Squad is subscribed to branch
lp:python-shell-toolbox.
--
Mailing list: https://launchpad.net/~yellow
Post to : [email protected]
Unsubscribe : https://launchpad.net/~yellow
More help : https://help.launchpad.net/ListHelp
