Code review comment for lp:~dcaro/terminator/fix-684340

Revision history for this message
Stephen Boddy (stephen-j-boddy) wrote :

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.

review: Abstain

« Back to merge proposal