Comment 2 for bug 58961

Revision history for this message
Jonathan Lange (jml) wrote :

OK. I've reviewed this and its mostly good -- thanks.

Here are my thoughts.
* The positional arguments (ala target) aren't 'flags' -- that implies some sort of boolean. Perhaps args, kwargs would be a better nomenclature than flags, pairs.
* Given that parseKVList doesn't do anything special with things that match 'modifier', it probably doesn't need to maintain a distinction between 'modifier' and 'value'. Given that 'value' already matches + and -, 'modifier' can be safely removed.
* You preserve case for flags and values, but not for keys. (lines 18, 21, 23 of parser.py)
* Pyparsing _might_ be overkill in this case. The obscure conditionals (good move on commenting them) seem to hint that it's not the best algorithm
* You should directly test parseKVList for strings like 'foo bar a=4 b=2 a=-3' and 'foo a=4 bar' etc
* Maybe it doesn't belong in this branch, but eventually cmd_add should call a method that has the signature add(actor, dexs=None, spds=None) and should called like a, kw = parseKVList(line); add(*a, **kw). Maybe. Maybe cmd_add should be that method. *shrug*

After these are fixed, go ahead and merge. Thanks again :)