Skip to content

Updates dice to use CommandModule#3

Open
mracine wants to merge 1 commit into
halibot-extra:masterfrom
mracine:commandmod-update
Open

Updates dice to use CommandModule#3
mracine wants to merge 1 commit into
halibot-extra:masterfrom
mracine:commandmod-update

Conversation

@mracine
Copy link
Copy Markdown

@mracine mracine commented Oct 29, 2019

Addresses halibot-extra/Meta#3
Updates to use CommandModule, also made some other modifications:

  • removed redundant "!roll" check
  • moved regex logic to _roll() function

This is no longer doing the list(filter(lambda))) that was being done before, as the internal _roll() function is checking the regex as the arguments are iterated over. I think the biggest change would mean now we would print an error message for each argument that is incorrect, instead of just ignoring it. So:
!roll 1d6 x"
will produce
4/6 = 4
You make a motion...

Of course we're not doing that nice OLOP anymore, which does nicely filter the list initially, but we're not rechecking the regex. Let me know if this makes sense or not.

Comment thread dice.py
if len(rolls) == 0:
self.reply(msg, body="You make a motion as if to roll some dice, but as you open your hands to throw them, only air escapes.")
return
def roll_(self, string, msg=None):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This naming scheme is potentially confusing, having a function called _roll and another called roll_. Maybe roll_cmd for the second? I can be convinced otherwise though.

Comment thread dice.py
coarse = msg.body.strip().split(" ")
cmd = coarse[0]
rolls = list(filter( lambda x: re.match(die_re, x), coarse[1:] ))
def _err(self):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think _err for this function name is too generic, as there is at least one other error case when the number of sides is 0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably makes more sense to make this a string constant anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants