yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

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.