parseKVList should parse Key-Value lists

Bug #58961 reported by Chris Halse Rogers
2
Affects Status Importance Assigned to Milestone
Joybot
Fix Released
Undecided
Unassigned

Bug Description

Currently, parseKVList is special-cased for Hit commands, so its name is misleading. Worse, other parts of Joybot (particularly !add) depend on this behaviour. Before fixing #58960, parseKVList should be fixed to actually parse Key-Value lists :)

Revision history for this message
Chris Halse Rogers (raof) wrote :

Committed to raof-devel. Please review.

Changed in joybot:
status: Unconfirmed → Fix Committed
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 :)

Changed in joybot:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.