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 merge methods for AppCommandContext and AppInstallationType #9829

Merged
merged 1 commit into from May 18, 2024

Conversation

JeronimusII
Copy link
Contributor

Summary

Fixes an issue with the merge() method of the AppCommandContext and AppInstallationType classes which would cause the "other" to always override the flags with True or False even when unspecified (None value).

Its caused by the getter methods being used which always turn None values into False so the condition other.guild is None for example never evaluates to True.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • 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, ...)

@Kile
Copy link
Contributor

Kile commented May 18, 2024

Just out of interest, why are not just using is False?

@JeronimusII
Copy link
Contributor Author

@Kile
When using is False the expression turns into something like:
guild = self.guild if other.guild is False else other.guild
Which essentially does the same as:
guild = self.guild or other.guild
Or when taking the getter methods into account:
guild = bool(self._guild) or bool(other._guild)

Now you cant override a True value in self._guild with a False in other._guild since True or False evaluates to True again.

The (I assume) expected behaviour is that the values of self are used unless explicitly overridden by the other values (not None).
Where the problem currently is that the getter methods turn a None value into False which causes the values to be always overridden with the ones from other (with False instead of None).
Which makes omitting a parameter in a decorator like @discord.app_commands.allowed_contexts() to keep the default value set in the constructor of discord.ext.commands.Bot (allowed_contexts parameter) not possible.

# Merging is similar to AllowedMentions where `self` is the base
# and the `other` is the override preference

# Creates a new AllowedMentions by merging from another one.
# Merge is done by using the 'self' values unless explicitly
# overridden by the 'other' values.

@Rapptz Rapptz merged commit b5ada0a into Rapptz:master May 18, 2024
8 checks passed
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.

None yet

3 participants