[PATCH 1 of 3] record: refactor the record extension's prompt()

[Steve Losh]

On Jan 24, 2010, at 9:37 AM, Patrick Mézard wrote:

> Le 05/01/10 01:44, Steve Losh a écrit :
>> # HG changeset patch
>> # User Steve Losh <steve at stevelosh.com>
>> # Date 1262652127 18000
>> # Node ID 1fa39ad1ca75699329f538aae321fcdcfbb5dfda
>> # Parent  f5e55f1ca92738458295ca9f82f31c54a89babbb
>> record: refactor the record extension's prompt()
>> 
>> This changeset refactors record.filterpatch.prompt() to be more flexible when
>> defining options.
> 
> I know you are not looking for formal patch review, but it is usually easier to fix style issues sooner than later, so here are a couple of remarks:
> 
>> diff --git a/hgext/record.py b/hgext/record.py
>> --- a/hgext/record.py
>> +++ b/hgext/record.py
>> @@ -284,34 +284,53 @@
>>             return resp_file[0]
>>         while True:
>>             resps = _('[Ynsfdaq?]')
>> -            choices = (_('&Yes, record this change'),
>> -                    _('&No, skip this change'),
>> -                    _('&Skip remaining changes to this file'),
>> -                    _('Record remaining changes to this &file'),
>> -                    _('&Done, skip remaining changes and files'),
>> -                    _('Record &all changes to all remaining files'),
>> -                    _('&Quit, recording no changes'),
>> -                    _('&?'))
>> +            all_choices = {
> 
> No underscores please
> 
> http://mercurial.selenic.com/wiki/BasicCodingStyle
> 

I was mainly trying to match the style of that file in particular.  In the first context line you can see a variable with an underscore that's already there.

Anyway, I can clean up the coding style once we've actually decided on a UI -- right now I'm just hacking together something that works so we can give any UI we come up with a test run to find the best interface.  The actual patch to add this shouldn't be *huge* so it won't be too hard to look over the entire thing and make sure everything is formatted correctly.

>> +                'yes': _('&Yes, record this change'),
>> +                'no': _('&No, skip this change'),
>> +                'skip': _('&Skip remaining changes to this file'),
>> +                'file': _('Record remaining changes to this &file'),
>> +                'done': _('&Done, skip remaining changes and files'),
>> +                'all': _('Record &all changes to all remaining files'),
>> +                'quit': _('&Quit, recording no changes'),
>> +                'help': _('&?'),
>> +            }
>> +            choices = (all_choices['yes'],
>> +                    all_choices['no'],
>> +                    all_choices['skip'],
>> +                    all_choices['file'],
>> +                    all_choices['done'],
>> +                    all_choices['all'],
>> +                    all_choices['quit'],
>> +                    all_choices['help'],)
> 
> I think you can build a tuple list first for ordering and build the dictionary from it.
> 
>>             r = ui.promptchoice("%s %s" % (query, resps), choices)
>> -            if r == 7: # ?
>> +            choice = [k for k in all_choices if all_choices[k] == choices[r]][0]
>> +            if choice == 'help':
>>                 doc = gettext(record.__doc__)
>>                 c = doc.find(_('y - record this change'))
>> -                for l in doc[c:].splitlines():
>> +                doc_lines = doc[c:].splitlines()
>> +                if not at_hunk:
>> +                    doc_lines = [l for l in doc_lines if not l.startswith('p - split')]
>> +                else:
>> +                    doc_lines = filter(
>> +                        lambda l: l.replace('(if you are currently at a hunk)', ''),
>> +                        doc_lines
>> +                    )
> 
> We never indent that way. What about:
> 
> docfilter = lambda l: l.replace('(if you are currently at a hunk)', '')
> doc_lines = filter(docfilter, doc_lines)
> 
> ?
> 
> Also, "p - split this hunk (if you are currently at a hunk)" is likely to be translated, so the prefix has to be defined differently.
> 
> --
> Patrick Mézard