-
Notifications
You must be signed in to change notification settings - Fork 395
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
Regex group support #130
base: develop
Are you sure you want to change the base?
Regex group support #130
Conversation
This could use some unit tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Jeong,
Thanks again for the contribution and for the tests. Did you have any luck running them? I noticed a small bug in the code that seems to be failing the tests when I try them out. My run is here: https://travis-ci.org/jtatum/slackbot/builds/221160048
The tests are a bit tricky to get working. I made a guide on how to do it. It's at https://github.com/lins05/slackbot/blob/develop/CONTRIBUTING.md#configure-tests
Even after correcting last_index to lastindex, I see the tests still fail with other issues. Please take a look and let me know what you think. If you have any problems getting them to run, ping me and I'll do my best to help!
slackbot/manager.py
Outdated
kwargs = m.groupdict() | ||
args = ( | ||
m.group(i) | ||
for i in range(1, m.last_index + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re match objects don't have a last_index.
* Fix typo on testing module's filename. * Fix category on testing message.
My mistake. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These updates are still failing functional tests. Please see my latest run at https://travis-ci.org/jtatum/slackbot/builds/223695036.
m.lastindex is None when pattern has no group.
slackbot/manager.py
Outdated
has_matching_plugin = True | ||
yield self.commands[category][matcher], to_utf8(m.groups()) | ||
yield self.commands[category][matcher], to_utf8(args), to_utf8(kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to_utf8
for now can only handle single string or list/tuple/set of strings, so you need to extend it to also handle dicts whose values are strings.
slackbot/manager.py
Outdated
|
||
if not has_matching_plugin: | ||
yield None, None | ||
yield None, None, None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FakeFakePluginManager also need to be updated to return a 3-tuple.
slackbot/manager.py
Outdated
args = tuple( | ||
m.group(i) | ||
for i in range(1, (m.lastindex or 0) + 1) | ||
if i not in matcher.groupindex.values() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe sth. like:
args = (v for i, v in enumerate(m.groups()) if (i + 1) not in pattern.groupindex.values())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth moving this to helper function in utils.py, e.g.
def extract_positional_and_named_groups(pattern, s):
if isinstance(pattern, basestring):
pattern = re.compile(pattern)
m = re.match(pattern, s)
kw = m.groupdict()
a = [v for i, v in enumerate(m.groups()) if (i + 1) not in pattern.groupindex.values()]
return a, kw
Seems the travis ci tests are still faling, check https://travis-ci.org/lins05/slackbot/builds/237900653?utm_source=github_status&utm_medium=notification |
Return dictionary, not list.
Sorry. I was a dumb. |
It support group name as kwargs for
bot.respond_to
method.Related issue: #118