Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates dice to use CommandModule #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mracine
Copy link

@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.

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
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.

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
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
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