Conversation
bbriggs
left a comment
bbriggs
left a comment
There was a problem hiding this comment.
I got through about 50% of it. I'll finish as time permits later this week.
Basically, parsing multiple arguments is hard. It gets harder when you want to branch many, many ways.
| def __init__(self, baseplate, lock): | ||
| super().__init__(baseplate, lock) | ||
| self.crypto_index = configparser.ConfigParser() | ||
| self.crypto_index.read('crypto_index.ini') |
There was a problem hiding this comment.
If you want to use a config file, you should check for presence first. I recommend making a configure method to handle this part.
| self.crypto_index.read('crypto_index.ini') | ||
| if 'crypto_index' not in self.crypto_index.sections(): | ||
| self.crypto_index['crypto_index'] = {} | ||
| self.crypto_index['crypto_index']['fsyms'] = 'BTC' |
There was a problem hiding this comment.
the Cryptowatch may have used the horribly cryptic fsyms and tsyms, but that doesn't mean we have to. It would probably benefit the user if we made these terms more clear and documented what they correspond to.
There was a problem hiding this comment.
resolved.
changed crypto_index to hodl
changed fsyms to convert_from
changed tsyms to convert_to
| else: | ||
| try: | ||
| query = message['text'].split()[1] | ||
| except: |
There was a problem hiding this comment.
Since you're splitting and looking for an index here, the exception you're almost certainly going to raise is an IndexError, which is what I would catch. Upon catching the index error, I'd then return a helpful message indicating as much.
try:
query: message['text'].split()[1]
except IndexError:
self.reply(message, "Not enough arguments or something", opts)
| self.reply(message, "Invalid query", opts) | ||
| self.reply(message, self._lookup_symbol(query), opts) | ||
|
|
||
| def _parse_multi_commands(self, message_list): |
There was a problem hiding this comment.
You're falling into the same trap we fell into with the proto-legobot back in 2013-14. This pattern didn't scale well for us back then and probably won't in this case either.
There was a problem hiding this comment.
'splain this one more plz
There was a problem hiding this comment.
We tried to write something along a similar pattern where we'd basically split the message and then branch on each option. It ended up getting messy, which was what drove us to the plugin model.
Using argparse for positional arguments or splitting hodl into another lego might be the way to go given how many branches we're starting to accumulate.
There was a problem hiding this comment.
Resolved by moving hodl to its own lego
2 participants
Please review and suggest interface changes.
!help crypto explains the new functions more in depth.