Conversation
This add a args feature, for example if your arg is called `argreply`
You can do: `?reply Hello, read the following: {argreply}
lorenzo132
requested review from
ModmailTest,
StephenDaDev,
Taaku18,
martinbndr and
sebkuip
lorenzo132
dismissed stale reviews from StephenDaDev and martinbndr
via
fed5044
StephenDaDev
added
the
changelog
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if name not in self.bot.args: | ||
| embed = create_not_found_embed(name, self.bot.args.keys(), "Arg") |
There was a problem hiding this comment.
The args command uses direct name lookup without resolving through aliases like the snippet command does with _resolve_snippet. This creates an inconsistency where snippets can be accessed via their aliases but args cannot. Consider adding a _resolve_arg method similar to _resolve_snippet to provide consistent behavior.
cogs/modmail.py
Outdated
|
|
||
| When `{prefix}args` is used by itself, this will retrieve | ||
| a list of args that are currently set. `{prefix}args name` will show what the | ||
| arg point to. |
There was a problem hiding this comment.
The description for the args command docstring says "arg point to" which is grammatically incorrect. It should be "arg points to" (singular verb agreement).
| arg point to. | |
| arg points to. |
| To create an arg: | ||
| - `{prefix}args add arg-name A value.` | ||
|
|
||
| You can use your arg in a reply with `{arg-name}`. |
There was a problem hiding this comment.
The documentation states "You can use your arg in a reply with {arg-name}" but doesn't mention that args only work in certain reply commands (reply, freply, fareply, fpreply, fpareply) and not in others (areply, preply, pareply). Consider updating the documentation to clarify which commands support args to avoid user confusion.
| You can use your arg in a reply with `{arg-name}`. | |
| You can use your arg in supported reply commands (` | |
| reply`, `freply`, `fareply`, `fpreply`, `fpareply`) with `{arg-name}`. Args are | |
| not available in `areply`, `preply`, or `pareply`. |
| if not self.bot.args: | ||
| embed = discord.Embed( | ||
| color=self.bot.error_color, | ||
| description="You dont have any args at the moment.", |
There was a problem hiding this comment.
Inconsistent contraction usage: "dont" should be "don't" with an apostrophe.
| description="You dont have any args at the moment.", | |
| description="You don't have any args at the moment.", |
| **self.bot.args, | ||
| channel=ctx.channel, | ||
| recipient=ctx.thread.recipient, | ||
| author=ctx.message.author, |
There was a problem hiding this comment.
In the freply, fareply, fpreply, and fpareply commands, args are added before the built-in variables (channel, recipient, author). This means that if an arg is named "channel", "recipient", or "author", it will be overridden by the built-in variables, potentially causing unexpected behavior. Consider either adding args after the built-in variables (so built-in variables take precedence) or documenting this behavior and warning users not to use reserved names.
Signed-off-by: lorenzo132 <50767078+lorenzo132@users.noreply.github.com>
sebkuip
left a comment
sebkuip
left a comment
There was a problem hiding this comment.
There are some issues in this PR that need to be addressed beforehand.
| """ | ||
|
|
||
| if self.bot.args: | ||
| msg = UnseenFormatter().format(msg, **self.bot.args) |
There was a problem hiding this comment.
There is no check for total message length after applying args. If I make an arg that's very long (I tested with a long part of lorem ipsum) then make a long reply plus the arg going over the limit, I just get an "unknown error". The bot logs show it goes over the limit.
As per the docs (screenshot below), the total length of an embed cannot exceed 6000 characters, and the descritpion may only be 4096 characters long. Please add a check to give a human error.
There was a problem hiding this comment.
Here is the result of a long message
| description=f'`{name}` has been renamed to "{value}".', | ||
| ) | ||
| else: | ||
| embed = create_not_found_embed(name, self.bot.args.keys(), "Arg") |
There was a problem hiding this comment.
For readability, this might be better fit as a guard clause at the start of the function. So
if name not in self.bot.args:
return await ctx.send(embed=create_not_found_embed(name, self.bot.args.keys(), "Arg"))
or alternatively
if name not in self.bot.args:
embed = create_not_found_embed(name, self.bot.args.keys(), "Arg")
return await ctx.send(embed=embed)
| {prefix}args add name value | ||
| ``` | ||
| """ | ||
| if name in self.bot.args: |
There was a problem hiding this comment.
This should also check that there is no conflict with the keywords from formatreply (channel, recipient and author). Currently this causes a silent fail without error. See screenshots.
| """ | ||
|
|
||
| if self.bot.args: | ||
| msg = UnseenFormatter().format(msg, **self.bot.args) |
There was a problem hiding this comment.
Replace this formatter to be consistent with how formatreply works (using self.bot.formater.format())
This is done correctly in the formatreply command.
|
|
||
| def process_help_msg(self, help_: str): | ||
| return help_.format(prefix=self.context.clean_prefix) if help_ else "No help message." | ||
| return help_.replace("{prefix}", self.context.clean_prefix) if help_ else "No help message." |
There was a problem hiding this comment.
Why is this being changed out? As far as I'm aware, the format() call was working just fine. If really needed, please elaborate why this is changed out.
This add a args feature, for example if your arg is called
argreplyYou can do: `?reply Hello, read the following: {argreply}