Merge lp:~evfool/checkbox/secondfix786924 into lp:checkbox

[Robert Roth]

Replaced the current string.lowercase[optionindex] option character choice in the CLI interface with the first letter of the option lowercased for each option, so we will have a more natural y) yes n) no instead of a) yes b) no optionlist (fixes bug #786924)

I have changed the fix to iterate over the characters of the option until it finds one that is not used yet in the keys. If all the characters of the option are used, it will find a letter from the alphabet which is not used yet. This should solve the problem for options starting with the same letter, eg "Yes/No/Not tested" will have the options y/n/o.

[Marc Tardif (cr3)]

The count variable can be assigned any integer value between 0 and len(findkey)-1. However, considering len(findkey) > len(option), this code is likely to result in a IndexError exception:

  key = option[count].lower()

Also, instead of using a count variable, you might like to try something like:

  for key in option.lower() + string.lowercase:
    if key not in self.keys:
      break

review: Needs Fixing

[lp:~evfool/checkbox/secondfix786924]

915. By Robert Roth on 2011-05-24

Fixed indexerror in case of falling back to alphabet

[Robert Roth (evfool)]

Yes, I've missed that, as I first implemented iterating over the options chars and added the alphabet fallback later. Fixed it based on your suggestions. I wanted to avoid break, but in this case it seems to be a bit better. I've also added another variable to avoid converting to lowercase and concatenating the strings in each step. See the branch for the changes.

[Marc Tardif (cr3)]

First, you don't need to declare and assign the count variable anymore. Second, for i in expression doesn't evaluate the expression on each iteration, so you don't need the findkey variable either. On a last note, I think that the for loop is pretty clear to not need such a large comment. I'll leave that to your discretion though.

review: Needs Fixing

[lp:~evfool/checkbox/secondfix786924]

916. By Robert Roth on 2011-05-24

Fixed small problems based on suggestions

917. By Robert Roth on 2011-05-25

Unnecessary comment removed

[Robert Roth (evfool)]

Fixed the minor problems based on Marc's comments.

[Marc Tardif (cr3)]

Looks good, merging!

review: Approve

=== modified file 'checkbox_cli/cli_interface.py'
--- checkbox_cli/cli_interface.py	2010-11-25 18:15:08 +0000
+++ checkbox_cli/cli_interface.py	2011-05-25 06:33:30 +0000
@@ -145,7 +145,9 @@
 
     def add_option(self, option, key=None):
         if key is None:
-            key = string.lowercase[len(self.keys)]
+            for key in option.lower()+string.lowercase:
+                if key not in self.keys:
+                    break
         self.keys.append(key)
         self.options.append(option)