Skip to content

Use re.findall() instead of re.search() in message_listener_matches - #387

Merged
seratch merged 2 commits into
slackapi:mainfrom
albeec13:albeec13-fix-issue-386
Jul 1, 2021
Merged

seratch merged 2 commits into
slackapi:mainfrom
albeec13:albeec13-fix-issue-386

Conversation

@albeec13

@albeec13 albeec13 commented Jul 1, 2021 •

Copy link
Copy Markdown
Contributor

Using re.findall() provides better match results in cases where the programmer would like to match the same group multiple times in a single chat message, which would previously only return a single result. Behavior for existing use cases, such as matching two different groups, continues to return a tuple that matches the old behavior or re.search().groups() for backward compatibility. This fixes issue #386

(Describe the goal of this PR. Mention any related Issue numbers)

Category (place an x in each of the [ ])

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Document pages under /docs
  • Others

Requirements (place an x in each [ ])

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run ./scripts/install_all_and_run_tests.sh after making the changes.

Using re.findall() provides better match results in cases where the programmer would like to match the same group multiple times in a single chat message, which would previously only return a single result. Behavior for existing use cases, such as matching two different groups, continues to return a tuple that matches the old behavior or re.search().groups() for backward compatibility
@seratch seratch added this to the 1.7.0 milestone Jul 1, 2021
@seratch

seratch commented Jul 1, 2021

Copy link
Copy Markdown
Contributor

Hi @albeec13, thanks a lot for submitting this PR as well! Before reviewing the changes, can I ask you to add new test patterns in the following test files? You can use the scripts under the scripts directory to easily run tests.

If creating new test files having test_message_ prefix is easier for you, that's also fine!

@codecov

codecov Bot commented Jul 1, 2021 •

Copy link
Copy Markdown

Codecov Report

Merging #387 (26337a8) into main (4731a28) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #387   +/-   ##
=======================================
  Coverage   91.59%   91.60%           
=======================================
  Files         167      167           
  Lines        5378     5384    +6     
=======================================
+ Hits         4926     4932    +6     
  Misses        452      452           
Impacted Files Coverage Δ
...listener_matches/async_message_listener_matches.py 100.00% <100.00%> (ø)
...ssage_listener_matches/message_listener_matches.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4731a28...26337a8. Read the comment docs.

@albeec13

albeec13 commented Jul 1, 2021

Copy link
Copy Markdown
Contributor Author

Hi @albeec13, thanks a lot for submitting this PR as well! Before reviewing the changes, can I ask you to add new test patterns in the following test files? You can use the scripts under the scripts directory to easily run tests.

* https://github.com/slackapi/bolt-python/blob/v1.6.1/tests/scenario_tests/test_message.py

* https://github.com/slackapi/bolt-python/blob/v1.6.1/tests/scenario_tests_async/test_message.py

If creating new test files having test_message_ prefix is easier for you, that's also fine!

I've pushed new test cases and validated that they fail before and succeed after the changes in this PR. Should be good to go.

@seratch seratch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perfect! Looks great to me 👍

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants