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

Switch to regex for banphrase regexes #1469

Merged
merged 4 commits into from
Sep 26, 2021
Merged

Conversation

RAnders00
Copy link
Contributor

@RAnders00 RAnders00 commented Sep 19, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable
  • Documentation in docs/ or install-docs/ was updated, if applicable
  • I have tested all changes

Fixes #1468

@RAnders00 RAnders00 marked this pull request as draft September 19, 2021 22:00
@zneix
Copy link
Contributor

zneix commented Sep 25, 2021

Did a little bit of testing on my own:

  1. Some basic banphrases without regex weren't affected and seem to work fine.
  2. Baisc regex banphrases with what re has been supporting so far works as well. I tested this with banphrases on my own pajbot instance
  3. Banphrase regex mentioned in Consider migrating from "re" to "regex" library for matching regular expressions #1468 does work with this change.
  4. I'm unsure whether some other advanced regexes, in e.g. #forsen would break. Maybe @Leppunen or pajlada can help with this one. Though, as far as I can tell based on my lidl research, nothing should break. See point 3. https://github.com/alvations/sacremoses/issues/76#issuecomment-590047152

@RAnders00 RAnders00 marked this pull request as ready for review September 25, 2021 14:00
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Tested on a bunch of production banphrases, and things seem to work as expected. LGTM 👍

@pajlada pajlada enabled auto-merge (squash) September 26, 2021 09:46
@pajlada pajlada merged commit 6ce6e45 into master Sep 26, 2021
@pajlada pajlada deleted the regex-lib-for-banphrases branch September 26, 2021 09:56
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.

Consider migrating from "re" to "regex" library for matching regular expressions
3 participants