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

Add support to jsk sync for clearing application commands #229

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

clari7744
Copy link
Contributor

@clari7744 clari7744 commented Mar 20, 2024

Usage remains largely the same, but prefixing a target with - will switch to upserting an empty payload
- alone or -$ can both be used to clear global commands
-., -*, or -{guild_id} all simply clear the commands for the expected target

Rationale

Currently, if I want to clear my commands (my use case, btw, is clearing from the beta bot so they don't interfere with the public bot), I have to manually type bot.tree.clear_commands(guild=guild/None), so this just makes it easier to wipe commands from a locale.

On a similar note, I wanted to also ask - thoughts on a way to replicate

CommandTree.copy_global_to(guild=guild)
await CommandTree.sync(guild=guild)

with jsk sync? I was thinking something like +{target} could handle that for guild ids/*/., and be ignored for $?

Summary of changes made

I created a second set that holds the guilds to be cleared, and changed the cleaned guilds list to be a tuple of [id, clear?], then loop through and set payload to an empty list when clear is True

Checklist

  • This PR changes the jishaku module/cog codebase
    • These changes add new functionality to the module/cog
    • These changes fix an issue or bug in the module/cog
    • I have tested that these changes work on a production bot codebase
    • I have tested these changes against the CI/CD test suite
    • I have updated the documentation to reflect these changes
  • This PR changes the CI/CD test suite
    • I have tested my suite changes are well-formed (all tests can be discovered)
    • These changes adjust existing test cases
    • These changes add new test cases
  • This PR changes prose (such as the documentation, README or other Markdown/RST documents)
    • I have proofread my changes for grammar and spelling issues
    • I have tested that any changes regarding Markdown/RST syntax result in a well formed document

Usage remains largely the same, but prefixing a target with `-` will switch upserting an empty payload
`-` alone or `-$` can both be used to clear global commands
`-.`, `-*`, or `-{guild_id}` all simply clear the commands for the expected target
Comment on lines +217 to +222
active_set = guilds_set
if target[0] == '-':
active_set = clear_guilds_set
target = target[1:]
if target in ('', '$'):
active_set.add(None)
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like there's gotta be a cleaner way of doing this. The plain method of parsing was probably fine when it was just a few special cases, but maybe it needs a bit of a do-over if we want it to handle removals and transferring of commands. Not sure of the approach though.

Copy link
Contributor Author

@clari7744 clari7744 Apr 15, 2024

Choose a reason for hiding this comment

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

Yeah, I considered a couple things but really not sure of the cleanest method here. We could use a match/case, but that's not compatible with <3.10, and there's still the issue of separating guilds-to-clear and guilds-to-add. If you have a general idea of what a cleaner method would be I could look into fleshing out/implementing something like it, but can't think of anything right now.

Vioshim added a commit to Vioshim/jishaku that referenced this pull request Oct 5, 2024
@Vioshim Vioshim mentioned this pull request Oct 5, 2024
13 tasks
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.

2 participants