[PATCH 1 of 2] record: refactored prompt choices to clean code and offer better choices

timeless timeless at gmail.com
Thu Feb 23 05:23:08 CST 2012


So. Localized command letters can be an interesting disaster. -
Especially for users likely to switch between en and some other
language that uses Latin letters (it's a *different* disaster for non
Latin languages...).

On 2/23/12, A. S. Budden <abudden at gmail.com> wrote:
> On 22 February 2012 23:34, Matt Mackall <mpm at selenic.com> wrote:
>> On Mon, 2012-02-20 at 13:30 +0000, A. S. Budden wrote:
>>> # HG changeset patch
>>> # User A. S. Budden <abudden at gmail.com>
>>> # Date 1329682649 0
>>> # Node ID ea6086ad210db327591965ad58f9e656313ce2fe
>>> # Parent  41fc1e078d6822502277ac80325ec3db99707a6f
>>> record: refactored prompt choices to clean code and offer better choices
>>>
>>> This patch tidies up the prompt choice code in order to allow options to
>>> be
>>> presented based on relevance.  The original implementation had a hard
>>> coded
>>> array of options and a hard-coded string with the selection characters.
>>> Addition of new options would require manual renumbering of all of the
>>> if/elif
>>> lines as it relied on the order of the two variables.  It was also
>>> impractical
>>> to remove individual entries depending on context.  This patch replaces
>>> the two
>>> main variables ('resps' and 'choices') with one ('choices').  Each tuple
>>> in the
>>> choices array contains an internal name (the first entry) that is used in
>>> the
>>> if/elif lines and a string containing an ampersand that is used in the
>>> help
>>> text and as a means of determining which character should be offered by
>>> ui.promptchoice.
>>
>> Gosh. How does this interact with translation? See here:
>>
>> http://mercurial.selenic.com/bts/issue3082
>>
>> The most confusing part of this is how the output for '?' changes, due
>> to it being generated from the prompt strings rather than pulled out of
>> the doc string. That's the sort of thing that should be tackled in a
>> separate patch.
>
> Okay.  I can go back to using the doc string if you'd prefer.  My main
> aim with this specific patch was to be able to remove individual
> entries from the choice list: if the help string is still generated
> from doc text then it becomes very difficult (or at least rather
> nasty) to change the text depending on which options are actually
> being presented.
>
> Given that the presented options [Ynsfdaq?] are now generated from the
> translated help text, I'd assumed (okay, maybe I need to learn how to
> test i18n stuff...) that in other languages the options presented (and
> used by ui.promptchoice) would change according to which character was
> preceded by an '&' and hence the translations would work correctly.  I
> guess it also depends on whether you'd prefer to always display
> "[Ynsfdaq?]" regardless of the language (and ensure that the
> translations mark the correct letter with an ampersand) or offer a
> language-specific prompt like "[Jnüdeav?]" (for Ja, Nein, Überspringe,
> Datei, Erledigt, Alle, Verlassen) - apologies to anyone who speaks
> German if I've got those wrong!
>
>>> diff --git a/hgext/record.py b/hgext/record.py
>>> --- a/hgext/record.py
>>> +++ b/hgext/record.py
>>> @@ -276,37 +276,49 @@
>>>          if skipfile is not None:
>>>              return skipfile, skipfile, skipall
>>>          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'),
>>> -                    _('&?'))
>>> -            r = ui.promptchoice("%s %s" % (query, resps), choices)
>>> +            choices = [
>>> +                    ('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')),
>>> +                    ('?', _('&?, display help'))]
>>
>> Let's lose the uppercase here. Mercurial uses lowercase in messages
>> generally, and we just end up doing extra steps. Case conversion is also
>> an i18n trap as different languages have different rules (ie German).
>
> Sounds like a good plan: I'd thought about doing this anyway (due to
> the lowercase messages rule), but I was trying to change as little as
> practical!
>
>> I guess the goal here is to have a fixed mapping for indices when we
>> remove entries.
>
> That and to make the code a bit more readable/maintainable: I have a
> tendency to dislike code like "if r == 0: # yes" rather than "if
> results[r] == 'yes'" as the former relies on up-to-date comments and
> you have to go through and re-enumerate large amounts of code if you
> ever add a new entry in the middle.
>
>> This is a bit awkward, because it introduced a bunch of
>> non-i18n-marked strings so people who audit for such things will have to
>> figure out what's up here repeatedly.
>
> True.  If I were writing in C I'd probably use a 'proper' enum and a
> table linking enums to strings (so you got a enumerated set of
> constants to refer to and a set of strings to display).  I've never
> found a clean way to do this in python: I dislike 'magic' index
> constants and, as you say, internal strings can confuse translators
> etc.
>
>> But if we did:
>>
>>  choices = [ old list of strings ]
>>  choices = enumerate(choices)
>>  choice.pop(5)
>>
>> ..now choice[0] is the "old" fixed index, despite the pop.
>>
>> Another possibility is to keep the existing resps/choices data and do:
>>
>>  newresps = list(resps)
>>  if foo:
>>   newresps.pop(5) # remove prompt we don't want
>>   choices.pop(5) # from both lists
>>  ...do our prompt...
>>  r = resps.index(newresps[r]) # map back to original numbering
>
> It's your code, so if you prefer this I'm happy to change it to
> something like this with numerical if statements that have to all be
> modified if a new entry is added.  I'll just repeat my reservations
> and let you decide!
>
>>> +            # Extract the parts of 'choices' as separate lists
>>> +            promptchoices = map(lambda x: x[1], choices)
>>> +            results = map(lambda x: x[0], choices)
>>
>> The cool kids use list comprehensions now rather than map and lambda:
>>
>>  promptchoices = [x[1] for x in choices]
>>  results = [x[0] for x in choices]
>>
>> But the wizards do something like this:
>>
>>  results, promptchoices = zip(*choices)
>>
>> (Wizards are too smart for their own good.)
>
> Looking back at my patch, I've no idea why I used map rather than a
> list comprehension... I'll change it!
>
>>> +            # Get the key characters based on the character after the
>>> +            # ampersand and make the first option in the list upper case
>>> +            # as an indication that it's the default
>>> +            keychars = map(lambda x: x.split('&',1)[1][0].lower(),
>>> +                    promptchoices)
>>
>> keychars = [x[x.index('&') + 1].lower() for x in promptchoices]
>
> Yes.
>
>>> +            keychars[0] = keychars[0].upper()
>>> +            # Prompt the user
>>> +            userprompt = '%s [%s]' % (query, ''.join(keychars))
>>> +            r = ui.promptchoice(userprompt, promptchoices)
>>>              ui.write("\n")
>>> -            if r == 7: # ?
>>> -                doc = gettext(record.__doc__)
>>> -                c = doc.find('::') + 2
>>> -                for l in doc[c:].splitlines():
>>> -                    if l.startswith('      '):
>>> -                        ui.write(l.strip(), '\n')
>>> +            if r == results.index('?'):
>>
>> That seems like a convoluted way of writing:
>>
>> if results[r] == '?':
>
> Good point (although implementing this will depend on whether we
> switch back to numerical checks obviously).
>
>>> +                # Generate a help string based on the text passed
>>> +                # to ui.promptchoice
>>> +                doc = '\n'.join(map(lambda x, y: x.lower() + ' - ' + \
>>> +                        y.replace('&', ''), keychars, promptchoices))
>>> +                ui.write(doc + '\n')
>>
>> - use list comprehension
>> - use "%s - %s\n"
>> - use ''.join()
>
> Okay.
>
>>
>>>                  continue
>>> -            elif r == 0: # yes
>>> +            elif r == results.index('yes'): # yes
>>
>> These comments are now redundant.
>
> Okay, I'll remove them if we don't go back to numerical indices
> (although shall I keep '(record remaining)' and '(skip remaining)' for
> clarity?).
>
>>
>>>                  ret = True
>>> -            elif r == 1: # no
>>> +            elif r == results.index('no'): # no
>>>                  ret = False
>>> -            elif r == 2: # Skip
>>> +            elif r == results.index('skip'): # Skip
>>>                  ret = skipfile = False
>>> -            elif r == 3: # file (Record remaining)
>>> +            elif r == results.index('file'): # file (Record remaining)
>>>                  ret = skipfile = True
>>> -            elif r == 4: # done, skip remaining
>>> +            elif r == results.index('done'): # done, skip remaining
>>>                  ret = skipall = False
>>> -            elif r == 5: # all
>>> +            elif r == results.index('all'): # all
>>>                  ret = skipall = True
>>> -            elif r == 6: # quit
>>> +            elif r == results.index('quit'): # quit
>>>                  raise util.Abort(_('user quit'))
>>>              return ret, skipfile, skipall
>>>
>>> diff --git a/tests/test-record.t b/tests/test-record.t
>>> --- a/tests/test-record.t
>>> +++ b/tests/test-record.t
>>> @@ -666,14 +666,14 @@
>>>    diff --git a/subdir/f1 b/subdir/f1
>>>    1 hunks, 1 lines changed
>>>    examine changes to 'subdir/f1'? [Ynsfdaq?]
>>> -  y - record this change
>>> -  n - skip this change
>>> -  s - skip remaining changes to this file
>>> -  f - record remaining changes to this file
>>> -  d - done, skip remaining changes and files
>>> -  a - record all changes to all remaining files
>>> -  q - quit, recording no changes
>>> -  ? - display help
>>> +  y - Yes, record this change
>>> +  n - No, skip this change
>>> +  s - Skip remaining changes to this file
>>> +  f - Record remaining changes to this file
>>> +  d - Done, skip remaining changes and files
>>> +  a - Record all changes to all remaining files
>>> +  q - Quit, recording no changes
>>> +  ? - ?, display help
>>>    examine changes to 'subdir/f1'? [Ynsfdaq?]
>>>    abort: user quit
>>>    [255]
>
> Thanks for the feedback,
>
> Al
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>

-- 
Sent from my mobile device


More information about the Mercurial-devel mailing list