Merge lp:~dcaro/terminator/fix-684340 into lp:terminator/trunk
Proposed by
David Caro
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 1430 | ||||
Proposed branch: | lp:~dcaro/terminator/fix-684340 | ||||
Merge into: | lp:terminator/trunk | ||||
Diff against target: |
204 lines (+56/-39) 2 files modified
terminatorlib/config.py (+9/-0) terminatorlib/plugins/custom_commands.py (+47/-39) |
||||
To merge this branch: | bzr merge lp:~dcaro/terminator/fix-684340 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Stephen Boddy | Abstain | ||
Review via email: mp+87045@code.launchpad.net |
Description of the change
Hi,
I've added a delete method for the plugins config to enable 'cleaning' the plugins configuration before saving (maybe it would be better to add a key delete method to avoid deleting the entire config each time).
And added a 'position' field for each command config, so you can keep track of the order when loading the commands (maybe the config should keep track of the order of the properties so this 'position' field is not needed?).
David
To post a comment you must log in.
Hi David. A couple of comments:
1) You have unnecessary random line inserts and removals, i.e. lines 136-137, 141, 191, 198, & 201 particularly stand out, but there may be more I missed
2) The chunk of lines 40-72 a significant chuck of that is actually identical, but your changes have unnecessarily altered indentation.
3) It would be preferable if the two items (fix ordering, and fix deletion) were two seperate commits. Makes it easier to see whats going on.
Other than that the patch works and fixes the issue. I'm marking as abstain for now. I won't merge this directly, but when I find time I'll clean it up and place it into trunk.