Code review comment for lp:~garyvdm/bzr/loggraphviz

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

> Ascii Unicode
> # * # ○
> # | # │
> # * | # ○ │
> # | | # │ │
> # * | | # ○ │ │
> # |--- # ├─╯─╯
> # * # ○
>
>
> That ok. But when the graphs get a bit more complicated, it starts to fail.
>
> # + # ⊕
> # |--- # ├┄╮─╮
> # | : * # │ ┆ ○
> # | -|- # │ ╰┄┼┄╮
> # | | * # │ │ ○
> # | | | # │ │ │
> # ~ | | # ⊖ │ │
> # |- | | # ├─╮ │ │
> # | * | | # │ ○ │ │
> # |----- # ├─╯─╯─╯
> # * # ○
>
> The unicode versions made it much easier for me to debug problems.
>
>> And I get some *really* strange results in whatever font Thunderbird
>> tries to set as default.
>
> Was that not because I left out the # -*- coding: utf-8 -*- line? Do
> the unicode characters show fine in this mail?

I'm attaching the screenshot of the default font that Thunderbird is
using. Even stranger, it looks better, but still poorly aligned when
replying to your email. But it is completely unreadable in Vim. I did
eventually find a font where it looks ok (Deja Vu Sans Mono). But most
other fonts (Courier New especially) just show garbage.

I do understand how it could help you, but I'm curious if it doesn't
just add confusion for a lot of other people....

>
>> And all those lines are >80 chars as well.
>
> I did start trying to fit them in to < 80 chars, but this was time
> consuming, and for me, makes it harder to read.
>
> Here is a quote from PEP 8, which I feel applies:
>
>> Two good reasons to break a particular rule:
>>
>> (1) When applying the rule would make the code less readable, even for
>> someone who is used to reading code that follows the rules.
>

Couldn't you just put the graph by itself in a comment ahead of time? I
do this in bzrlib/tests/test_graph.py and it worked well for me.

If you did that, then you could even put both the ascii and unicode
versions. So you would have:

+ # branch lines should not cross over
+ # ascii unicode
+ # ~ ⊖
+ # |---- ├───╮
+ # | ~ │ ⊖
+ # | | │ │
+ # ~ | ⊖ │
+ # |-- | ├─╮ │
+ # | * | │ ○ │
+ # | |- │ ├─╯
+ # | * │ ○
+ # |- ├─╯
+ # * ○

+ self.assertComputed(
+ [('rev-f', 0, True, [(0, 0, 0, True), (0, 2, 3, True)]),
+ ('rev-e', 2, True, [(0, 0, 0, True), (2, 2, 2, True)]),
+ ('rev-d', 0, True, [(0, 0, 0, True), (0, 1, 2, True), (2,
2, 2, True)]),
+ ('rev-c', 1, None, [(0, 0, 0, True), (1, 1, 2, True), (2,
1, 2, True)]),
+ ('rev-b', 1, None, [(0, 0, 0, True), (1, 0, 0, True)]),
+ ('rev-a', 0, None, [])]
+ computed)

That allows:
a) Less that 80 chars
b) You don't have the artificial extra blank lines in the list output
c) Put the ascii first. At least when I look at it, Unicode is more
   likely to screw up the spacing, causing the ascii art to get
   incorrectly drawn.

I realize it doesn't give you a strict line-to-line correspondence of
which list entry corresponds to what graph entry. But as long as the
graph isn't very large it doesn't seem that hard to do.

If you really needed you could number the lines and put #1 at the end of
the list entries.

Or you could put the 'rev-f', etc, on the graph information.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzmo3AACgkQJdeBCYSNAAOBuwCgg/iDFYYGmrfzmBc/GKbD8X6s
pd4AoNOjWENORNjumB7vk3p8bxnd4RkX
=CnXB
-----END PGP SIGNATURE-----