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

Matt Mackall mpm at selenic.com
Tue Feb 28 17:16:57 CST 2012


On Tue, 2012-02-28 at 15:16 +0000, A. S. Budden wrote:
> # HG changeset patch
> # User A. S. Budden <abudden at gmail.com>
> # Date 1329682649 0
> # Node ID 18bd6bc67345de83db30e2ae1cb0d6fbe77f57ea
> # Parent  41fc1e078d6822502277ac80325ec3db99707a6f
> record: refactored prompt choices to clean code and offer better choices

I think I owe you an apology for making you wade into this particular
morass. Now that I've been looking at it, I see there's a lot that's
just wrong here to start with and fixing it up is hard:

- we're poking in the docs to generate help
- the docs (almost) duplicate the built-in prompt strings
- the prompt strings don't match normal capitalization style
- the prompt strings are normally only used/seen by third-party code
like THG which wraps ui objects
- the prompt characters themselves are translatable
- translated characters are not guaranteed to be one byte wide
- the translated characters must correspond with the translated prompt
strings
- single-character strings are just bad news for translators
- there's a bunch of fiddling with upper/lowercase that's i18n-naïve
- promptchoice itself is pretty i18n-naïve itself
- promptchoice returns an index, which means it's inherently hostile to
dynamic prompt choices

Given that there's a bunch of hard issues here with conflicting
solutions, the odds of a new contributor getting it all right in one
patch is approximately 0.0000000001%. And indeed the present patch take
a few steps forward and several steps back.

Any time you try to solve two problems at the same time, your odds of
getting it right shrink dramatically. Which is why I'm continuously
saying "one issue per patch, please".

I don't want to make fixing this particular disaster your burden, but I
also don't want to make things worse here by piling on more technical
debt. So probably, I'll just take your original patch, and we can try to
tackle the above issues one by one.


> 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.
> 
> diff --git a/hgext/record.py b/hgext/record.py
> old mode 100644
> new mode 100755
> --- a/hgext/record.py
> +++ b/hgext/record.py
> @@ -276,37 +276,42 @@
>          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 = [
> +                    ('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'))]
> +
> +            # Extract the parts of 'choices' as separate lists
> +            promptchoices = ['&%s - %s' % x for x in choices]
> +            keychars = ''.join([x[0] for x in choices])
> +            # Prompt the user
> +            userprompt = '%s [%s]' % (query, 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 choices[r][0] == '?':
> +                # Generate a help string based on the text passed
> +                # to ui.promptchoice.  Help text is the same as
> +                # promptchoices but without the ampersand.
> +                ui.write(''.join(['%s - %s\n' % x for x in choices]))
>                  continue
> -            elif r == 0: # yes
> +            elif choices[r][0] == 'Y': # yes
>                  ret = True
> -            elif r == 1: # no
> +            elif choices[r][0] == 'n': # no
>                  ret = False
> -            elif r == 2: # Skip
> +            elif choices[r][0] == 's': # skip one
>                  ret = skipfile = False
> -            elif r == 3: # file (Record remaining)
> +            elif choices[r][0] == 'f': # file (record remaining)
>                  ret = skipfile = True
> -            elif r == 4: # done, skip remaining
> +            elif choices[r][0] == 'd': # done, skip remaining
>                  ret = skipall = False
> -            elif r == 5: # all
> +            elif choices[r][0] == 'a': # all
>                  ret = skipall = True
> -            elif r == 6: # quit
> +            elif choices[r][0] == 'q': # quit
>                  raise util.Abort(_('user quit'))
>              return ret, skipfile, skipall
>  
> diff --git a/tests/test-record.t b/tests/test-record.t
> old mode 100644
> new mode 100755
> --- a/tests/test-record.t
> +++ b/tests/test-record.t
> @@ -666,7 +666,7 @@
>    diff --git a/subdir/f1 b/subdir/f1
>    1 hunks, 1 lines changed
>    examine changes to 'subdir/f1'? [Ynsfdaq?] 
> -  y - record this change
> +  Y - record this change
>    n - skip this change
>    s - skip remaining changes to this file
>    f - record remaining changes to this file
> _______________________________________________
> 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