Conversation
I do have a number of things to review, but for now this looks great! Though I will ask: please make sure this targets the
|
EDIT: you can retarget to |
I'll retarget when I get home from work |
for more information, see https://pre-commit.ci
This pr is now retargetted and updated to account for the new branch's requirements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I like this! I still need to test it, and do wonder if subcommands work correctly (I doubt it, honestly), but everything seems mostly solid.
molter/help.py
Outdated
self.show_aliases = show_aliases | ||
self.show_prefix = show_prefix | ||
self.embed_title = embed_title or "{username} Help Command" | ||
self.not_found_message = not_found_message or "Sorry! No command called `{cmd_name}` was found." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick: the phrasing for the not found default string is slightly unprofessional with the exclamation mark, but... yeah, that's a nitpick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, 2 objections to that.
One, exclamation marks are not unprofessional. Theyre frequently used in throughout professional settings in order to reduce end-user stress when something doesnt work. I believe you mean its not emotively neutral, which also leads into the second point.
Two, Snek as a whole is not professional, so the point is quite void.
""" | ||
out: dict[str, molter.MolterCommand] = {} | ||
|
||
for cmd in self._client.commands.values(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be assumed that every command here is a Molter command... or at least it may be a good idea just to add a filter for normal message commands just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding was MolterCommands were replacing snek's message commands, is this not the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a filter
async def _help_specific(self, ctx: dis_snek.MessageContext, cmd_name: str) -> None: | ||
cmds = await self._gather(ctx) | ||
|
||
if cmd := cmds.get(cmd_name.lower()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a naive approach and I think it won't work well with subcommands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be honest, i completely forgot message subcommands were a thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely works as help._gather
generates a dict of {cmd_resolved_name, cmd}
and the help_specific method is looking up by a resolved name (which the user would know commands by), so technically this should be the ideal route
for more information, see https://pre-commit.ci
Originally put on the wrong method, mb
Re-opened under #8 , as my jank handling of the earlier branch conflict was causing issues after applying the above changes. |
This is a basic implementation of a help command to tick off one of your points in #3.
Pre-merge utilisation of this is fairly simple
To modify it, pre-merge, i really think just overriding is the way to go. Post merge i would like to introduce some sort of client method to modify help.
This pr is dependant on dev-latest of dis-snake. Specifically NAFTeam/NAFF@af64022