[MERGE] Patience diff
jrydberg at gnu.org
Tue May 23 14:21:43 BST 2006
John A Meinel <john at arbash-meinel.com> writes:
> [comments on Aarons things]
John, when commenting on code, could you maybe try to keep the inlined
code to a minimum? I find it hard to navigate through your comments,
for two reason;
1) because of all the excessive inlined code, I'm having trouble
understanding exactly what you are commenting on.
When quoting, there is nothing wrong with minimizing the text.
Actually, it makes things easier to read. If people want the
read the whole thread, they can do just that in their own
mail reader. :)
2) are you commenting on the code above or below the comment?
You make for example this comment:
> +from bisect import bisect
> +from copy import copy
> +import difflib
> +import time
> +import os
> +import sys
> PEP8 here
I suppose you are commenting on the imports (ie, commenting the code
_above_ the comment).
> + result = 
> + k = lasts[-1]
> + while k is not None:
> + result.append((btoa[k], k))
> + k = backpointers[k]
> + result.reverse()
> + return result
> PEP8. Also, Bram seemed to like running self tests every time the module
> was imported. Which are what all these asserts are for.
> They should be fast, so I'm okay with keeping them, but I did add them
> to selftest, so it would be safe to get rid of them.
> +assert unique_lcs('', '') == 
> +assert unique_lcs('a', 'a') == [(0, 0)]
> +assert unique_lcs('a', 'b') == 
> +assert unique_lcs('ab', 'ab') == [(0, 0), (1, 1)]
> +assert unique_lcs('abcde', 'cdeab') == [(2, 0), (3, 1), (4, 2)]
> +assert unique_lcs('cdeab', 'abcde') == [(0, 2), (1, 3), (2, 4)]
> +assert unique_lcs('abXde', 'abYde') == [(0, 0), (1, 1), (3, 3), (4, 4)]
> +assert unique_lcs('acbac', 'abc') == [(2, 1)]
Where as here it seems that you are commenting on the code _below_ the
comment. (maybe with the exception of the PEP8 comment)
Or it might just be my jetlag that is causing brain damage. Anyhow, I
appreciate your comments, and I hope you take this the right way.
More information about the bazaar