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.

Sign up for GitHub

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

Jump to bottom

Migrate !source to /source (app_commands) #2446

Closed
wants to merge 17 commits into from
Closed

Migrate !source to /source (app_commands) #2446

wants to merge 17 commits into from

Conversation

Ibrahim2750mi
Copy link
Contributor

@Ibrahim2750mi Ibrahim2750mi commented Mar 6, 2023 •
edited
Loading

Closes #2022
Previously the error prompted because isinstance was returning False when an TagIdentifier object was compared to itself. Instead of checking that, we check if the source object issubclass of commands.Cog and pass the TagIdentifier to else.

Closes #2424
Screenshots:
2023-03-06_18-15_1
2023-03-06_18-16
2023-03-06_18-39

Ibrahim2750mi
Ibrahim2750mi commented Mar 6, 2023
View reviewed changes
bot/exts/info/source.py Outdated Show resolved Hide resolved
Ibrahim2750mi
Ibrahim2750mi commented Mar 6, 2023
View reviewed changes
bot/exts/info/source.py Outdated Show resolved Hide resolved
shtlrs
shtlrs requested changes Mar 8, 2023
View reviewed changes
Copy link
Member

@shtlrs shtlrs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good overall, just small things that need fixing

bot/converters.py Outdated Show resolved Hide resolved
bot/exts/info/source.py Outdated Show resolved Hide resolved
bot/converters.py Show resolved Hide resolved
bot/exts/info/source.py Outdated Show resolved Hide resolved
bot/exts/info/source.py Show resolved Hide resolved
shtlrs
shtlrs reviewed Mar 9, 2023
View reviewed changes
bot/converters.py Outdated Show resolved Hide resolved
shtlrs
shtlrs reviewed Mar 9, 2023
View reviewed changes
bot/exts/info/source.py Outdated Show resolved Hide resolved
Ibrahim2750mi
Ibrahim2750mi commented Mar 9, 2023
View reviewed changes
bot/exts/info/source.py Show resolved Hide resolved
bot/exts/info/source.py Outdated Show resolved Hide resolved
Ibrahim2750mi and others added 5 commits March 9, 2023 18:11
@Ibrahim2750mi
Merge branch 'main' into migration/source
e834c76
@Ibrahim2750mi
Merge branch 'main' into migration/source
f5b6d87
@Ibrahim2750mi
Merge branch 'main' into migration/source
70820b4
@Ibrahim2750mi
Implement reviews
a1820fa 
+ Edited embed title to `Prefix-based commad` for non app_commands
+ Made the source command guild only
+ Allowed `app_commands.Group` in the transformer
@Ibrahim2750mi
Add error handler for app_commands
4601217
@Ibrahim2750mi Ibrahim2750mi requested review from shtlrs and removed request for jb3 and Den4200 March 24, 2023 11:39
shtlrs
shtlrs requested changes Mar 24, 2023
View reviewed changes
bot/converters.py Outdated Show resolved Hide resolved
bot/exts/info/source.py Outdated Show resolved Hide resolved
bot/exts/backend/error_handler.py Show resolved Hide resolved
bot/exts/backend/error_handler.py Outdated Show resolved Hide resolved
bot/converters.py Outdated Show resolved Hide resolved
bot/converters.py Outdated Show resolved Hide resolved
bot/exts/backend/error_handler.py Show resolved Hide resolved
bot/exts/backend/error_handler.py Outdated Show resolved Hide resolved
bot/exts/backend/error_handler.py Show resolved Hide resolved
@Ibrahim2750mi
Implement shtlrs reviews
13aa29a 
+ Remove redundant return statement in transformer
+ Remove error handling in the command itself
+ Used qualified name of the command for the debug_message in error handler
+ Changed `appcommand_invoke_error` to `app_command_invoke_error` for incrementing error stats
@Ibrahim2750mi
Use ValueError instead of BadArgument
6059dd0
@Ibrahim2750mi
Add unittests
9bd7a2d 
+ unittest for returning None when the error is internally handled
+ unittest for TransformerError
+ uniitest for CommandInvokeError
+ unittest for general AppCommandError
@Ibrahim2750mi
Copy link
Contributor Author

I have also added unit tests for error handler so someone in future doesn't break them, or if they they will get notified.

@shtlrs
Copy link
Member

shtlrs commented Apr 6, 2023

Hey @Ibrahim2750mi, I'm sorry this took a while.
Here's a couple of things I noticed

  • It currently have duplicates for some command groups, like infractions
    image
    image

The second & third options point to the same command group.

  • We don't have autocomplete for subcommands of a certain group, for example the edit subcommand of the infraction group

image
However, it still works when i force that value, meaning when I do write infraction edit and press enter, it fetches it corretly
image

@Xithrius Xithrius added a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) p: 2 - normal Normal Priority a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) t: enhancement Changes or improvements to existing features s: needs review Author is waiting for someone to review and approve labels May 2, 2023
@Xithrius
Copy link
Member

Xithrius commented May 2, 2023 •
edited
Loading

This PR is now up for grabs, as discussed here.

@Xithrius Xithrius added up for grabs Available for anyone to work on and removed s: needs review Author is waiting for someone to review and approve labels May 2, 2023
@Canttuchdiz
Copy link
Contributor

I'd like to be assigned.

@Xithrius Xithrius removed the up for grabs Available for anyone to work on label May 2, 2023
@Xithrius
Copy link
Member

@Canttuchdiz Hello, will you be continuing updates to this PR?

@Xithrius Xithrius self-requested a review September 23, 2023 06:07
@Xithrius Xithrius added the up for grabs Available for anyone to work on label Mar 3, 2024
@wookie184 wookie184 removed their request for review March 25, 2024 19:39
@wookie184 wookie184 added the s: stale Has had no activity for a while label Mar 29, 2024
@wookie184
Copy link
Contributor

I think @hedyhli's proposed solution to #2022 is a better approach. Along with this, as this has quite a few merge conflicts, is stale, and there are still some existing bugs, I'm going to close the PR and suggest anyone wanting to work on #2424 makes a new one.

@wookie184 wookie184 closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shtlrs shtlrs shtlrs requested changes

Assignees
No one assigned
Labels
a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) p: 2 - normal Normal Priority s: stale Has had no activity for a while t: enhancement Changes or improvements to existing features up for grabs Available for anyone to work on
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make !source a slash command Incorrect handling of tags by the !source command
5 participants
@Ibrahim2750mi @shtlrs @Xithrius @Canttuchdiz @wookie184