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

fix: cog can still be MISSING #2313

Closed
wants to merge 3 commits into from
Closed

fix: cog can still be MISSING #2313

wants to merge 3 commits into from

Conversation

ChunkLightTuna
Copy link

@ChunkLightTuna ChunkLightTuna commented Jan 6, 2024

Summary

#2303 partially changed checks of MISSING to None with the assumption that cog would never be MISSING. Cog is MISSING for commands instantiated from @bot.slash_command().

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

@Dorukyum
Copy link
Member

Dorukyum commented Jan 7, 2024

I have missed a part of code, yes, but your changes affect the logic too. Will review shortly.

@Dorukyum Dorukyum changed the title fix: Fix command options instantiated from @bot.slash_command() fix: cog can still be MISSING Jan 7, 2024
Comment on lines -270 to +269
MISSING,
None,
)
if match_ is not MISSING and _check_command(subcommand, match_):
if match_ and _check_command(subcommand, match_):
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this has anything to do with the cog attribute being MISSING, the MISSING value here is used for matching commands.

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -298,17 +295,15 @@ def _check_command(cmd: ApplicationCommand, match: Mapping[str, Any]) -> bool:
falsy_vals = (False, [])
for opt in value:
cmd_vals = (
[val.get(opt, MISSING) for val in as_dict[check]]
[val.get(opt) for val in as_dict[check]]
Copy link
Member

Choose a reason for hiding this comment

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

Neither as_dict nor to_check will ever contain cog in this chunk of the code. I don't understand how this fixes the issue, MISSING was again used for matching commands.

Copy link
Author

Choose a reason for hiding this comment

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

I’ll need to test this more. Earlier when I had accidentally inverted the logic, it fixed the issue with non-cog commands, but now when properly treating None as a drop in replacement for MISSING it's again not retrieving options.

@Dorukyum
Copy link
Member

It seems your thesis was wrong and I'm fixing the actual issue in a different pull request. Thanks anyway.

@Dorukyum Dorukyum closed this Jan 22, 2024
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