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 issues with embed_requested() #2966

Merged
merged 3 commits into from Aug 30, 2019
Merged

Fix issues with embed_requested() #2966

merged 3 commits into from Aug 30, 2019

Conversation

DJtheRedstoner
Copy link
Contributor

@DJtheRedstoner DJtheRedstoner commented Aug 29, 2019

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

Remove if statement that causes embed_requested() not to always return the correct user set value.

Tobotimus
Tobotimus previously requested changes Aug 29, 2019
Copy link
Member

@Tobotimus Tobotimus left a comment

This will break embed_requested() in DMs, because PrivateChannels don't have a guild attribute.

The current behaviour is that the guild setting takes priority over the user setting, and as far as I can tell, this behaviour is intended. Perhaps we should open up a discussion on whether the embed setting should be personalised, or whether the guild setting should take priority?

@DJtheRedstoner
Copy link
Contributor Author

@DJtheRedstoner DJtheRedstoner commented Aug 29, 2019

What does need to be fixed is that the user setting should always affect help but only affects help run with [p]help and not help sent by other commands

@mikeshardmind
Copy link
Contributor

@mikeshardmind mikeshardmind commented Aug 29, 2019

What does need to be fixed is that the user setting should always affect help but only affects help run with [p]help and not help sent by other commands

I can't replicate the described issue. ctx.send_help() sent from a command actually invokes the same exact help logic as running [p]help commandname as a command with respect to the embed setting. (they both call: ctx.bot.send_help_for with the context preserved)

@DJtheRedstoner
Copy link
Contributor Author

@DJtheRedstoner DJtheRedstoner commented Aug 29, 2019

Screen Shot 2019-08-29 at 7 10 46 PM

It only works in a guild channel.

@mikeshardmind
Copy link
Contributor

@mikeshardmind mikeshardmind commented Aug 29, 2019

As Toby said,

The current behaviour is that the guild setting takes priority over the user setting, and as far as I can tell, this behaviour is intended. Perhaps we should open up a discussion on whether the embed setting should be personalised, or whether the guild setting should take priority?

Above image shows this, but these changes don't adequately handle this.

I'm also not sure we should change the priority here. Guild owners have a reason to expect help to look consistent within their own server.

@DJtheRedstoner
Copy link
Contributor Author

@DJtheRedstoner DJtheRedstoner commented Aug 29, 2019

I did not realize that the intended behaviour was guild having priority over user, but it does not make sense to me why user should only have priority over guild in help (and there IS a bug with this).

Is the intended priority Guild > User > Global?

My suggestion is to make the user setting ONLY apply in DM's

Make embed_requested()'s user settings only affect DM's
Copy link
Contributor

@mikeshardmind mikeshardmind left a comment

Much more acceptable way to handle this.

@mikeshardmind mikeshardmind merged commit ef3ac77 into Cog-Creators:V3/develop Aug 30, 2019
1 check passed
@mikeshardmind mikeshardmind added this to the 3.2.0 milestone Aug 30, 2019
jack1142 pushed a commit that referenced this issue Jan 17, 2020
- However, we are not changing the signature
  - This was previously special cased for reasons related to the older
  version of the help formatter we used and never re-evaluated for need.
  - We should leave the signature as is both for lack of breaking, and
  for potential future changes

// actually this was already done once in GH-2966 but got accidentally overwritten
@jack1142 jack1142 added the Type: Bug label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants