[PATCH 1 of 2] record: refactored prompt choices to clean code and offer better choices
A. S. Budden
abudden at gmail.com
Thu Feb 23 08:28:36 UTC 2012
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
More information about the Mercurial-devel
mailing list