[PATCH 1 of 2] record: refactored prompt choices to clean code and offer better choices
Matt Mackall
mpm at selenic.com
Wed Feb 22 17:34:30 CST 2012
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.
> 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).
I guess the goal here is to have a fixed mapping for indices when we
remove entries. 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. 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
> + # 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.)
> + # 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]
> + 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] == '?':
> + # 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()
> continue
> - elif r == 0: # yes
> + elif r == results.index('yes'): # yes
These comments are now redundant.
> 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]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
--
Mathematics is the supreme nostalgia of our time.
More information about the Mercurial-devel
mailing list