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

[RFC] [commands] custom default arguments #1849

Closed
wants to merge 4 commits into from

Conversation

khazhyk
Copy link
Collaborator

@khazhyk khazhyk commented Jan 26, 2019

Modeled slightly after Converters, allow specifying Converter-like class
for context-based default parameters. e.g.

class Author(ParamDefault):
  async def default(self, ctx):
    return ctx.author

async def my_command(ctx, user: discord.Member=Author):
  ...

@khazhyk khazhyk force-pushed the rfc-param-default branch 5 times, most recently from 89f537a to 9ef70d0 Compare January 26, 2019 21:30
@Gelbpunkt
Copy link
Contributor

Gelbpunkt commented Apr 22, 2019

@Rapptz any update/statement on this? Would make some discord.py code far prettier and seems a not-breaking change.
Referring to the example, currently it's:

async def my_command(ctx, user: discord.Member=None):
    user = user or ctx.author

which is ugly

@khazhyk
Copy link
Collaborator Author

khazhyk commented Apr 22, 2019

I might have time to finish this up this week/end

@khazhyk khazhyk changed the base branch from rewrite to master April 26, 2019 05:35
@khazhyk khazhyk force-pushed the rfc-param-default branch 2 times, most recently from b36e013 to 79c8cc7 Compare April 26, 2019 05:46
@khazhyk
Copy link
Collaborator Author

khazhyk commented Apr 26, 2019

  • Added example and docs
  • Fixed issue where non-specified converter type would default to type instead of str

Would appreciate others testing and comments.

@khazhyk khazhyk force-pushed the rfc-param-default branch 3 times, most recently from 4540528 to b113c8c Compare April 28, 2019 19:22
Copy link
Contributor

@Gelbpunkt Gelbpunkt left a comment

Choose a reason for hiding this comment

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

Does the job and is a great code readability enhancement

@Gelbpunkt
Copy link
Contributor

I may have found a bug?

from discord.ext.commands.default import Author

@bot.command()
async def testme(ctx, thing: discord.Member = Author):
    await ctx.send(thing.joined_at)

This raises AttributeError: type object 'Author' has no attribute 'joined_at'.
When I print dir(thing), it's all the proper attributes there, even joined_at

@khazhyk
Copy link
Collaborator Author

khazhyk commented May 5, 2019

I can't reproduce ( see https://canary.discordapp.com/channels/336642139381301249/381963689470984203/574499828172587038 and following) - the behavior your experiencing seems like the command framework isn't evaluating the customdefaults, does this behavior persist past restart?

@khazhyk
Copy link
Collaborator Author

khazhyk commented May 5, 2019

Ah - ctx.invoke() is what isn't working

ctx.invoke doesn't call converters at all - so also won't resolve defaults. what you probably want is bot.invoke(ctx) rather than ctx.invoke(command) (which do different things)

@Gelbpunkt
Copy link
Contributor

Was an issue on my end - the code proposed in the PR works fine. Thanks!

@khazhyk
Copy link
Collaborator Author

khazhyk commented May 8, 2019

Pushed an update to fix a missing isclass check which could cause some commands with defaults to break... whoops

@Rapptz Rapptz added the needs review This PR will be reviewed in the future label May 9, 2019
@Gelbpunkt
Copy link
Contributor

I also found this to look weird on command.signature. Here's what it looks like:
[argument=<class 'discord.ext.commands.default.Author'>]

Is there a way to prettify that or do I have to write my own signature generator?

@NCPlayz
Copy link
Contributor

NCPlayz commented May 12, 2019

The classes should probably define their own __repr__ to prettify that

@Gelbpunkt
Copy link
Contributor

Yea, I know that. Not sure if they want to write a new repr for the "prepacked" defaults and what it would be, though. [argument=Author] isn't very user-friendly either...

@thecaralice
Copy link

@khazhyk I created a PR to your fork to fix the problem with signature khazhyk#2

@khazhyk
Copy link
Collaborator Author

khazhyk commented May 12, 2019

Does the default HelpCommand already have special casing for Converter? It would make sense to treat it similarly to Converter (which does not override repr just for HelpCommand)

@thecaralice
Copy link

Type hints (including converters) are not displayed in help lol

@thecaralice
Copy link

I tested your example from docs and there are some name errors

@avayert
Copy link

avayert commented Jun 19, 2019

I don't like the duplicate name for Guild (and to a certain extent Channel) as it already exists as discord.Guild. discord.Guild also has converter support built into it in the commands API which would make this inconsistent and a little confusing, even when using the namespaced default.Guild.

I don't currently know how or if it would even make sense to attach the defaults to said class, but a simple fix could be to rename them to CurrentGuild and CurrentChannel, which I find makes the signature more comprehensible to read.

@ioistired
Copy link
Contributor

Attaching the defaults to the classes would be far worse IMO, as you'd end up with signatures like "x: discord.Guild = discord.Guild" which to me.is very confusing. I think it would be better to rename them to Current* yes.

@khazhyk
Copy link
Collaborator Author

khazhyk commented Jun 24, 2019

I don't currently know how or if it would even make sense to attach the defaults to said class, but a simple fix could be to rename them to CurrentGuild and CurrentChannel, which I find makes the signature more comprehensible to read.

Doing CurrentGuild and CurrentChannel makes sense to me, done.

@TheRockettek
Copy link

TheRockettek commented Jul 1, 2019

Because it needs to be reviewed. The only tag there is my dude

@sairam4123
Copy link

Is this dead?

@thecaralice
Copy link

It’s still waiting for review

@sairam4123
Copy link

sairam4123 commented Sep 5, 2020

Because it has been 1 year
that's why, I am asking!

@khazhyk
Copy link
Collaborator Author

khazhyk commented Sep 5, 2020

Rebased this. I recall some people claimed issues between customdefaults and some of the newer argument types (greedy, etc.). Is that the case/still the case? Is there an example that behaves weirdly?

@sairam4123
Copy link

Let me test the code.

@sairam4123
Copy link

sairam4123 commented Sep 6, 2020

Just a suggestion, instead of Author, you could do, CurrentAuthor would be nice to see.

@sairam4123
Copy link

sairam4123 commented Sep 6, 2020

I got an error when trying to use Greedy with this. @khazhyk

Ignoring exception in command my_command:
Traceback (most recent call last):
  File "F:\PyCharm Python Works\OpenCityBot\discord.py\discord\ext\commands\core.py", line 86, in wrapped
    ret = await coro(*args, **kwargs)
  File "F:\PyCharm Python Works\OpenCityBot\discord.py\test.py", line 14, in my_command
    await ctx.send(member.display_name)
AttributeError: type object 'Author' has no attribute 'display_name'

Here's the code I used.

@bot.command()
async def my_command(ctx, member: commands.Greedy[discord.Member] = Author, channel: discord.TextChannel = CurrentChannel, guild = CurrentGuild):
    await ctx.send(member.display_name)
    await ctx.send(channel.name)
    await ctx.send(guild.name)

Without Greedy it works super fine, so, check why the Greedy isn't working, I'll check the others as well, like Optional and Channel.

Tested Union[discord.Member, discord.User] works fine
Tested Optional[discord.Member] works fine

To fix:

  1. Greedy.

@khazhyk
Copy link
Collaborator Author

khazhyk commented Apr 18, 2021

Just a suggestion, instead of Author, you could do, CurrentAuthor would be nice to see.

The reason I went for "Author" instead of "CurrentAuthor" is that Author I think should be unambiguous, vs. Channel/Guild, which would be ambiguous. And stuff like CurrentUser also could be misunderstood. So CurrentAuthor seems too wordy, imo, but does look slightly more consistent. I'm indifferent, do others have thoughts on this?

khazhyk and others added 4 commits April 19, 2021 02:16
Modeled slightly after Converters, allow specifying Converter-like class
for context-based default parameters. e.g.

```py
class Author(CustomDefault):
  async def default(self, ctx):
    return ctx.author

async def my_command(ctx, user: discord.Member=Author):
  ...
```

Also adds a few common cases (Author, Channel, Guild) for current
author, ...
With this change CustomDefaults display nicer in help commands and
in command.signature
[khazhyk: rebased on latest and resolved conflicts in _get_converter]
[khazhyk: removed list handling, adjusted message]
@khazhyk
Copy link
Collaborator Author

khazhyk commented Apr 19, 2021

@sairam4123 figured out and fixed the issue with Greedy, thanks! That should work now.

@khazhyk
Copy link
Collaborator Author

khazhyk commented Apr 19, 2021

I also added a commit from @sgtlaggy which I thought was useful, although there could be some bikeshedding to do here. In particular, I'm not so sure that having a member variable to override is the best api.

one option, which I don't think works in this current PR, is if the defaultargument was both a converter and a default, then detect if the default is (also) a converter, and use that, e.g.

class MyDefault(DefaultArgument, MyConverter):
  ...

@Tari-dev
Copy link
Contributor

Tari-dev commented Mar 6, 2022

If we are going to add this couldn't we support attachment with ext.commands?
Also we could support for slash-ext hybrid?

@khazhyk
Copy link
Collaborator Author

khazhyk commented Mar 6, 2022

Sorry, as evidenced by my lack of touching this for a while, I'm not really motivated to push this to completion. I still think something like this is a good idea, and if anyone else wants to implement something like this (or polish this code up for the lib), I would be happy to see it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review This PR will be reviewed in the future
Projects
None yet
Development

Successfully merging this pull request may close these issues.