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

[Core] Change all toggles in core commands to setters #4460

Open
wants to merge 5 commits into
base: V3/develop
Choose a base branch
from

Conversation

Dav-Git
Copy link
Contributor

@Dav-Git Dav-Git commented Oct 1, 2020

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

This changes all toggles in core commands to be bool setters to be consistent with

  • The rest of core
  • Other core cogs (e.g. Warnings)

I also updated the wording in the docstrings for these commands to reflect that they are not toggles anymore.

@Jackenmen
Copy link
Member

Do you have an interest in expanding this to other cogs, for example Mod's [p]modset?

@Jackenmen Jackenmen added Category: Bot Core Type: Enhancement Something meant to enhance existing Red features. labels Oct 1, 2020
@Jackenmen Jackenmen added this to the 3.4.1 milestone Oct 1, 2020
@Dav-Git
Copy link
Contributor Author

Dav-Git commented Oct 1, 2020

Yes. The goal is to slowly convert all of core to make it uniform.

For the sake of simplicity I though it would be best to split into seperate PRs per cored cog. (Hence I did warnings seperately) but if you want one big PR I can definitely do that as well.

@Drapersniper
Copy link
Contributor

Drapersniper commented Oct 13, 2020

So I'd like to state my official stance on this.

I don't think we should restrict functionality here, we should instead allow all commands like this to behave as both toggle AND setters doing the following.

 @embedset.command(name="global")
    @checks.is_owner()
    async def embedset_global(self, ctx: commands.Context):
    async def embedset_global(self, ctx: commands.Context, enabled: bool = None):
        """
        Toggle the global embed setting.
        Set the global embed setting.
        This is used as a fallback if the user
        or guild hasn't set a preference. The
        default is to use embeds.
        """
        if enabled is None:
        	enabled = not (await self.bot._config.embeds())
        await self.bot._config.embeds.set(enabled)
        await ctx.send(
            _("Embeds are now {} by default.").format(_("disabled") if enabled else _("enabled"))
        )

@Kowlin
Copy link
Member

Kowlin commented Dec 5, 2020

Seems we could go for some unification here on this field with setters and toggles. When discussing other unrelated things the idea popped up of unifying setting commands, in a way that they work both with a toggle and a boolean setter.

@Dav-Git
Copy link
Contributor Author

Dav-Git commented Dec 6, 2020

If I understand your comment correctly the thought is to have an optional bool argument that can set a value and otherwise default to a toggle? That sounds like a decent idea to me. I'd like to see more comments on this before I make changes to the PR though. Personally I am in favor.

@bobloy
Copy link
Contributor

bobloy commented Dec 6, 2020

I use this strategy often in Fox cogs, but sometimes no value is "reset to default" instead of toggle when it's not a bool but a multi select.

Overall I'm in favor.

@Jackenmen Jackenmen modified the milestones: 3.4.4, 3.4.5 Dec 15, 2020
@Dav-Git
Copy link
Contributor Author

Dav-Git commented Jan 7, 2021

I'm sorry for taking this long but I completely forgot about the suggestions here.

@Jackenmen Jackenmen modified the milestones: 3.4.6, 3.4.7 Jan 17, 2021
@Jackenmen Jackenmen modified the milestones: 3.4.13, 3.5.0 Jun 25, 2021
@Jackenmen Jackenmen modified the milestones: 3.5.0, 3.6.0 Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Type: Enhancement Something meant to enhance existing Red features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants