launchpad-reviewers team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Review: Approve code*

Another great contribution Ivo, thank you.
Suggestions follow.

#8
+    lowercase_args = False
Just an idea, and just a naming doubt: it seems that what we want to express here is the concept of a command with case insensitive args, and that converting them to lowercase is just an implementation detail. Maybe you could use a `case_insensitive_args` class attribute instead?


#97 and #104
if len(words) > 0:
Isn't it more "pythonic" to just check `if words:`?


#106
+                        if command_names[first]:
+                            words = words.lower()
+                        words = words.split(' ')
                     commands.append((first, words))
This is really a personal opinion, and maybe I am wrong, but I think the code could be more readable if written as::
    case_insensitive_args = command_names[first]
    if case_insensitive_args:
        words = words.lower()
    commands.append((first, words.split(' ')))



-- 
https://code.launchpad.net/~ivo-kracht/launchpad/bug-425934/+merge/110786
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.