[PATCH 1 of 2] record: refactored prompt choices to clean code and offer better choices
timeless
timeless at gmail.com
Thu Feb 23 11:23:08 UTC 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