Skip to content

Fix for rules greedy parsing freezing#2310

Merged
ChrisLovering merged 11 commits into
mainfrom
rules-fix
Oct 27, 2022
Merged

Fix for rules greedy parsing freezing#2310
ChrisLovering merged 11 commits into
mainfrom
rules-fix

Conversation

@ionite34
Copy link
Copy Markdown
Contributor

@ionite34 ionite34 commented Oct 25, 2022

Command works the same as the original specifications, bug is fixed for now.

img

@ionite34 ionite34 requested review from jb3 and mbaruh as code owners October 25, 2022 04:45
@MarkKoz MarkKoz added t: bug Something isn't working p: 1 - high High Priority a: information Related to information commands: (doc, help, information, reddit, site, tags) labels Oct 25, 2022
Comment thread bot/exts/info/information.py Outdated
Comment thread bot/exts/info/information.py Outdated
Comment thread tests/bot/exts/info/test_information.py Outdated
@onerandomusername
Copy link
Copy Markdown
Contributor

Here is a hotpatch if you're interested:

from disnake.ext.commands.view import StringView
from disnake.ext.commands.errors import ArgumentParsingError

_original_get_quoted_word = StringView.get_quoted_word

def get_quoted_word(self:StringView):
    try:
        _original_get_quoted_word(self)
    except ArgumentParsingError:
        raise RuntimeError

StringView.get_quoted_word = get_quoted_word

@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Oct 25, 2022

Err yes, that would probably be better. Trying to make the command work for both actual and test inputs is kinda wack, and properly modifying the test seems like a non-trivial amount of effort.

@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Oct 25, 2022

There seems to be code that handles ArgumentParsingError, so indisriminantely raising RuntimeError in its place seems like it could mess up the parsing of other things

https://github.com/Rapptz/discord.py/blob/01915fbc095a72ffd2231c72b7c0892a377a477c/discord/ext/commands/core.py#L698-L705

@onerandomusername
Copy link
Copy Markdown
Contributor

There seems to be code that handles ArgumentParsingError, so indisriminantely raising RuntimeError in its place seems like it could mess up the parsing of other things

https://github.com/Rapptz/discord.py/blob/01915fbc095a72ffd2231c72b7c0892a377a477c/discord/ext/commands/core.py#L698-L705

Yep, it would. Oops.

@ionite34
Copy link
Copy Markdown
Contributor Author

There seems to be code that handles ArgumentParsingError, so indisriminantely raising RuntimeError in its place seems like it could mess up the parsing of other things

Rapptz/discord.py@01915fb/discord/ext/commands/core.py#L698-L705

Yeah the monkey patch seems to no longer infinitely loop but breaks string parsing in most other places as well:

image

Is there something else we can do for patching?

@ionite34
Copy link
Copy Markdown
Contributor Author

@ionite34 ionite34 requested a review from MarkKoz October 25, 2022 13:57
@ionite34 ionite34 changed the title Temporary fix for rules greedy parsing freezing Fix for rules greedy parsing freezing Oct 25, 2022
Comment thread bot/exts/info/information.py Outdated
Comment thread bot/exts/info/information.py
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

I think the change in behaviour should be reverted. If we want to change it it can be done in a separate PR after discussing it, but it's out of scope for this PR given the original behaviour was intended

@ionite34
Copy link
Copy Markdown
Contributor Author

I think the change in behaviour should be reverted. If we want to change it it can be done in a separate PR after discussing it, but it's out of scope for this PR given the original behaviour was intended

I think that makes sense yeah, usually there's some natural messages after the rules. I reversed the changes in 8f68607 (#2310)

@ChrisLovering
Copy link
Copy Markdown
Member

I think the change in behaviour should be reverted. If we want to change it it can be done in a separate PR after discussing it, but it's out of scope for this PR given the original behaviour was intended

Ah right, we should definitely add a comment there explaining that use case then, as it currently just looks wrong to anyone who isn't aware of that.

@onerandomusername
Copy link
Copy Markdown
Contributor

New much simpler fix in 71fdd6c (#2310) as discovered by @ChrisLovering in https://canary.discord.com/channels/267624335836053506/675756741417369640/1034385695940296704

would you please provide the context for this link that isn't public?

@ionite34
Copy link
Copy Markdown
Contributor Author

New much simpler fix in 71fdd6c (#2310) as discovered by @ChrisLovering in canary.discord.com/channels/267624335836053506/675756741417369640/1034385695940296704

would you please provide the context for this link that isn't public?

It's just this diff that chris posted:

🕙 09:39:18 ❯ git diff
diff --git a/bot/exts/info/information.py b/bot/exts/info/information.py
index 2592e093..253c24fc 100644
--- a/bot/exts/info/information.py
+++ b/bot/exts/info/information.py
@@ -524,7 +524,7 @@ class Information(Cog):
         await self.send_raw_content(ctx, message, json=True)
 
     @command(aliases=("rule",))
-    async def rules(self, ctx: Context, *args: Optional[str]) -> Optional[Set[int]]:
+    async def rules(self, ctx: Context, *, args: Optional[str]) -> Optional[Set[int]]:
         """
         Provides a link to all rules or, if specified, displays specific rule(s).
 
@@ -541,7 +541,7 @@ class Information(Cog):
             for rule_keyword in rule_keywords:
                 keyword_to_rule_number[rule_keyword] = rule_number
 
-        for word in args:
+        for word in args.split():
             try:
                 rule_numbers.append(int(word))
             except ValueError:

@ionite34 ionite34 requested a review from wookie184 October 25, 2022 15:31
@shtlrs
Copy link
Copy Markdown
Contributor

shtlrs commented Oct 26, 2022

@ionite34 @ChrisLovering here's the - original issue where you'll find more details and context in the discussion / PR about the actual behavior

@Xithrius Xithrius added s: needs review Author is waiting for someone to review and approve a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) labels Oct 26, 2022
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

Tested, lgtm

@ChrisLovering ChrisLovering merged commit dbf3dcb into main Oct 27, 2022
@ChrisLovering ChrisLovering deleted the rules-fix branch October 27, 2022 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: information Related to information commands: (doc, help, information, reddit, site, tags) p: 1 - high High Priority s: needs review Author is waiting for someone to review and approve t: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants