Merge lp://staging/~frankban/python-shelltoolbox/helpers into lp://staging/python-shelltoolbox
Status: | Merged |
---|---|
Merged at revision: | 14 |
Proposed branch: | lp://staging/~frankban/python-shelltoolbox/helpers |
Merge into: | lp://staging/python-shelltoolbox |
Diff against target: |
1062 lines (+802/-147) 3 files modified
setup.py (+1/-1) shelltoolbox/__init__.py (+430/-114) tests.py (+371/-32) |
To merge this branch: | bzr merge lp://staging/~frankban/python-shelltoolbox/helpers |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gary Poster (community) | Approve | ||
Review via email:
|
Description of the change
== Changes ==
- Added several helper functions:
- bzr_whois
- file_append
- file_prepend
- generate_ssh_keys
- get_su_command
- join_command
- mkdirs
- ssh
- user_exists
- Added `environ` context manager and updated `su` to use it.
- Changed `apt_get_install` helper: now the function uses environ to run dpkg
in non-interactive mode.
- Changed `get_user_home` to handle non-existent users
(returning a default home directory)
- Updated the `install_
placeholders.
- Updated `cd` context manager: yield in a try/finally block.
- Fixed `grep` helper.
== Tests ==
python tests.py
.......
-------
Ran 53 tests in 0.124s
OK
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:
lines. remove( line)
lines. remove( line)
pass
if line in lines:
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:
except ValueError:
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: su_command( ...)). Maybe an example would help.
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_
I'm a bit concerned about what you are doing here in get_value_ from_line: '=')[1] .strip( '"\' ')
return line.split(
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