Merge lp:~blr/launchpad/bug-1334577-verbose-diff into lp:launchpad
Status: | Merged |
---|---|
Merged at revision: | 17590 |
Proposed branch: | lp:~blr/launchpad/bug-1334577-verbose-diff |
Merge into: | lp:launchpad |
Diff against target: |
379 lines (+208/-55) 3 files modified
lib/lp/code/mail/codereviewcomment.py (+87/-13) lib/lp/code/mail/tests/test_codereviewcomment.py (+120/-41) versions.cfg (+1/-1) |
To merge this branch: | bzr merge lp:~blr/launchpad/bug-1334577-verbose-diff |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Approve | ||
William Grant (community) | Needs Fixing | ||
Review via email:
|
Commit message
Check for inline comments associated with patch headers, hunk headers (hunk context lines) and hunk lines, and discard irrelevant bits in branch merge proposal mail.
Description of the change
Summary
Emails notifications of inline comments on a merge proposal are currently sent with the entirety of the diff, which is needlessly verbose and can make it difficult to find the relevant code. This branch provides a re-implementation of the code responsible for this and discards patches and diff hunks that do not have associated inline comments.
Proposed fix
Check for comments associated with patch headers, hunk headers (hunk context lines) and hunk lines, and discard irrelevant bits.
Implementation details
Currently the diff text is represented as a simple list. In order to have a structural representation of the diff, parse the diff text with bzrlib.patches. Ideally bzrlib.patches would provide line numbers, but as it does not wgrant and blr decided a less intrusive approach would be better than submitting a MP for bzrlib, even if it makes the mailer code more verbose.
LOC Rationale
More lines of code here to reduce lines of code elsewhere (email!). I think we're breaking even.
Tests
./bin/test -c -m lp.code.
Demo and Q/A
Running the tests locally is the best option, unless you have an MTA configured in your dev environment. I'm hoping we can test this with genuine emails on staging?
Lint
Lint free
This looks almost right now, but there seems to be a minor issue or two left over from William's earlier review, and I spotted what looks like a logic bug that you'll want to fix.