add game channel regex function #135
Conversation
instead of having to run the :level command, it will look for 'lvl' or 'level' in a message, and then show the level thing instead. it also checks it against what channel it was sent in, so person wont have to specify what base. i tried it on a private server, and it assigned the variables well.
|
Take a look at the Travis build errors and fix the flake8 issues |
i removed a few whitespaces, added some and added async def on_message(self, message: Message):
removed/added whitespace and took ':Message' out of 'async def on_message(self, message: Message): ' plswork
took 'self' out of 'async def on_message(self, message):' plswork
There was a problem hiding this comment.
Thank you very much for the contribution. I love the idea, and thank you for going ahead and implementing it!
There are a few little problems with this pull request, though, and I'll just list them here to be helpful.
You've created an on_message function, but there's already a function with that name on line 341 (323 before your changes). Python will only remember one of them, so the other one's functionality will be removed. Instead of defining the function again, try adding your changes to the existing function.
In addition to that, here are some other issues (again, quite minor and shouldn't take too long to remedy -- but if you need help, please ask!):
EDIT: You added a commit before I had a chance to finish writing my review; so everything's going to look a little out of order on GitHub -- and some things will be marked as "outdated" and collapsed when they're still relevant, but all my comments are still there and if you need help or don't understand what I'm saying do ask!
plswork moved my actual function down to the bottom with all the other regex functions fixed the match error
added a space after # added 2 blank lines after my code
There was a problem hiding this comment.
Thank you for making the changes. I'm really glad it's passing now and it's awesome that it is! I just wanted to note down some minor improvements that aren't required but just some notes for discussion. This pull request can be merged even without these changes, of course.
as suggested by joker314
i think this is it
will shortly modify my actual code
Co-Authored-By: maishams <43148755+maishams@users.noreply.github.com>
|
Oh, and the regex should probably catch alternate notation; so 13 1 (covered), 13.1, L13C1, l13c1, so on. |
|
It may be helpful if the regex only fired if the message were along the lines of "can I have a hint on..." or "I need some help on..." to avoid it popping up too much and obstructing discussion. |
|
Change the regex to |
Co-Authored-By: maishams <43148755+maishams@users.noreply.github.com>
There was a problem hiding this comment.
I think we should discuss the regular expression in a little more detail. A brief overview of my issues are:
- Matches words beginning with "L" even if they aren't related to levels
- Does not match unless at the beginning of line
- Matches "Lev X Y" and "LL X Y" even though these aren't typical abbreviations (although this is difficult... because I can't really imagine a context where this would be used and not refer to a level. Let me know what you think)
- Matches "Level XY" despite this notation being ambiguous and prone to false-positives
I really like everything in general, these are just tiny nit-picks!
| @@ -63,6 +63,10 @@ def __init__(self, bot: Bot): | |||
| r"^.*\bwhat\b.*\belite\b.*\bemail\b.*$", | |||
| re.IGNORECASE | |||
| ) | |||
| self.game_level_regex = re.compile( | |||
| r"^(le?v?e?l?).*?(\d{1,2}).*?(\d{1,2})\b.*$", | |||
There was a problem hiding this comment.
Okay, so this regular expression has changed a lot. It matches this, which I think it shouldn't:
Lol, remember the 2018 intake?
:level 20 18
This would maybe be acceptable, but in addition, the command will report out-of-range errors back to the user, in the form of a spammy message. We need to suppress these error messages if the command was invoked via this pattern match rather than a direct invocation of the command.
I think, also, in 5451798 we accidentally removed the .* prefixing the pattern for "level". This is bad because you can no longer use inline phrases like
I'm struggling with level 4.3
Maybe we ought to remove the ^ anchor at the beginning of the match, as that seems more readable than adding a .*.
Also, notice that "Lol" matches. This probably isn't a good idea. Maybe we should only match "L" on its own. "Lev" probably shouldn't be matched either, but "lvl" and "level" should be.
This change will resolve some of the above:
Also, "2019" shouldn't match. There should be a digit-boundary between each number, so that "20 19" is okay, but three-digit numbers should not work.
Here is a regular expression to consolidate the above requests:
| r"^(le?v?e?l?).*?(\d{1,2}).*?(\d{1,2})\b.*$", | |
| r"(le?v?e?l|l(?=\b|\d))\D*(\d{1,2})\D+(\d{1,2})\b.*$", |
Matches
- LxCy
- lvl x y
- lvl x.y
- levl x y
- lvel x y
- level x y
- lvl x challenge y
Does not match:
- lev x y
- love x y
- level xy
- level 01 04 (note leading zeros)
We should maybe consider allowing the last one.
Additionally, you use a capturing group to match for "level". I haven't changed it here because it would require changing some of your other code, but you never use this result so maybe we should use a non-capturing group (?:...) so as to only match what we need.
I really like how this has all turned out and am open to thoughts on the above!
Co-Authored-By: maishams <43148755+maishams@users.noreply.github.com>
Co-Authored-By: maishams <43148755+maishams@users.noreply.github.com>
Co-Authored-By: maishams <43148755+maishams@users.noreply.github.com>
|
As a heads up @maishams @justanotherfailure, I'm going to be closing this soon if merge conflicts aren't resolved. The PR has been open for over a month. |
instead of having to run the :level command, it will look for 'lvl' or 'level' in a message, and then show the level thing instead. it also checks it against what channel it was sent in, so person wont have to specify what base. i tried it on a private server, and it assigned the variables well.