[PATCH 1 of 2] graph: in hgrc specify line width for main branch

Martin Geisler mg at aragost.com
Mon Feb 6 08:20:45 UTC 2012


Constantine <theaspect at gmail.com> writes:

> Martin, Linus said "Just for fun". Are You think resending simple
> patch eight times funny?

Sorry about that -- we (core developers) often forget that it's not fun
to submit patches again and again... some people really appreciate the
feedback, but others have better things to do.

Overall, we want really pretty code in Mercurial and so the bar is a
little high for getting patches accepted.

> If my contribution worth somesing, you can fix it yourself, otherwise
> throw it away. But bear in mind, "graph" visualisation in hgweb
> completely broken: try to compare high-branched tree with tortoisehg's
> version.
>
> 2012/2/2 Martin Geisler <mg at aragost.com>
>
>> Constantine Linnick <theaspect at gmail.com> writes:
>>
>> > # HG changeset patch
>> > # User Constantine Linnick <theaspect at gmail.com>
>> > # Date 1327235726 -25200
>> > # Node ID 537733973603cb4f974f01aecd51923da17e314f
>> > # Parent  878bc4a62a735a1624e9b69c672d5fb5074d4855
>> > graph: in hgrc specify line width for main branch
>> >
>> > You can specify width to visually distinguish main branch (trunk)
>> > on hgweb's graph page. Settings format is branch_name.width = value,
>> > where width in px e.g.:
>> > [graph]
>> > default.width = 3
>> >
>> > diff -r 878bc4a62a73 -r 537733973603 mercurial/graphmod.py
>> > --- a/mercurial/graphmod.py   Thu Jan 19 14:31:05 2012 -0600
>> > +++ b/mercurial/graphmod.py   Sun Jan 22 19:35:26 2012 +0700
>> > @@ -18,6 +18,7 @@
>> >  """
>> >
>> >  from mercurial.node import nullrev
>> > +import re
>> >
>> >  CHANGESET = 'C'
>> >
>> > @@ -67,7 +68,7 @@
>> >          parents = set([p.rev() for p in ctx.parents() if p.node() in
>> include])
>> >          yield (ctx.rev(), CHANGESET, ctx, sorted(parents))
>> >
>> > -def colored(dag):
>> > +def colored(dag, repo):
>> >      """annotates a DAG with colored edge information
>> >
>> >      For each DAG node this function emits tuples::
>> > @@ -83,6 +84,21 @@
>> >      seen = []
>> >      colors = {}
>> >      newcolor = 1
>> > +    config = dict()
>>
>> There is another empty dict just above and it uses {}. I'm not sure
>> which I prefer myself, but it's important to be consistent. You
>> should therefore use {} here too so that you keep the coding style
>> consistent.
>
> Here:
> http://www.selenic.com/pipermail/mercurial-devel/2012-January/036816.html
> Mark said "No bug fixes, no spelling fixes, no coding style fixes, no
> whitespace fiddling, nothing."

I meant that I think you should use '{}' above to keep the overall style
more consistent. That's all.

(Btw, his name is Matt, not Mark.)

>> +    for key, val in repo.ui.configitems('graph'):
>> > +        if '.' not in key:
>> > +            continue
>> > +        branch, setting = key.rsplit('.',1)
>>
>> We use PEP 8 formatting in Mercurial. Please remember to run the full
>> test suite before submitting patches -- test-check-code-hg.t should
>> have caught the missing spaces around ','.
>
> I've run All tests. Your particular test case:
>
> ~/workspace/mercurial-repo/tests$ ./run-tests.py test-check-code-hg.t
> .
> # Ran 1 tests, 0 skipped, 0 failed.

Okay, I'm sorry the check-code script didn't catch this mistake.

>> > +                (setting == "width" and not re.match('[0-9]{1,2}',
>> val))):
>>
>> Please don't use regular expressions for something that can be
>> checked with simpler string functions:
>
> I test not digit, I test digit between 0 and 99. Small regexp simplier
> then two tests

I did see that you check for a number in the range 0..100. I think
that's best done with a simple

  0 < int(val) < 100

Here I would actually just check that 0 < int(val) and avoid the
arbitrary restriction on the line width. Unless it isn't arbitrary? I
guess the graph look really weird with a 100px wide line. I guess it
also looks pretty bad with a 30px line width?

-- 
Martin Geisler

aragost Trifork
Professional Mercurial support
http://www.aragost.com/mercurial/customer-projects/



More information about the Mercurial-devel mailing list