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

Option Redesign (fix for #513) #863

Closed
wants to merge 42 commits into from
Closed

Option Redesign (fix for #513) #863

wants to merge 42 commits into from

Conversation

EmreTech
Copy link
Contributor

@EmreTech EmreTech commented Jan 27, 2022

Summary

This PR fixes #513 by redesigning how Options are set. This also documents the Option class.

TODO List for this PR to be merged:

  • Update the option decorator to reflect how Options are set now
  • Fix options defined in the slash_command kwarg
  • Document Option

Checklist

  • 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
  • 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, ...)

@Lulalaby Lulalaby enabled auto-merge (squash) January 27, 2022 02:35
@Lulalaby Lulalaby added this to the v2.0 milestone Jan 27, 2022
@VincentRPS VincentRPS added bug Something isn't working Merge with squash priority: low Low Priority status: awaiting review Awaiting review from a maintainer labels Jan 27, 2022
auto-merge was automatically disabled January 27, 2022 04:28

Head branch was pushed to by a user without write access

@Lulalaby
Copy link
Member

@EmreTech what's the current state of this

@EmreTech
Copy link
Contributor Author

@EmreTech what's the current state of this

I've tested it, and it should be done now. I'm just waiting for reviews.

@Middledot
Copy link
Member

This seems to break if parentheses are in the option description.

@client.slash_command()
async def dis_is_test(ctx, channel: discord.Option(discord.TextChannel, description="Is a channel? (yes)")):
    await ctx.respond(channel.mention)

Value that is passed into gettype: TextChannel, description='Is a channel? (yes)' instead of only TextChannel

@EmreTech
Copy link
Contributor Author

This seems to break if parentheses are in the option description.

@client.slash_command()
async def dis_is_test(ctx, channel: discord.Option(discord.TextChannel, description="Is a channel? (yes)")):
    await ctx.respond(channel.mention)

Value that is passed into gettype: TextChannel, description='Is a channel? (yes)' instead of only TextChannel

Working on it, thanks for telling me

@EmreTech
Copy link
Contributor Author

This seems to break if parentheses are in the option description.

@client.slash_command()
async def dis_is_test(ctx, channel: discord.Option(discord.TextChannel, description="Is a channel? (yes)")):
    await ctx.respond(channel.mention)

Value that is passed into gettype: TextChannel, description='Is a channel? (yes)' instead of only TextChannel

Done.

@Lulalaby Lulalaby enabled auto-merge (squash) January 29, 2022 03:31
Lulalaby
Lulalaby previously approved these changes Jan 29, 2022
@Lulalaby
Copy link
Member

Holy fuck, this got big.
Gonna review over the day.

@krittick krittick removed this from In progress in v2.0 Mar 7, 2022
@krittick krittick added this to In progress in v2.1 via automation Mar 7, 2022
@krittick krittick modified the milestones: v2.0, v2.1 Mar 7, 2022
@Dorukyum Dorukyum marked this pull request as draft March 8, 2022 18:14
@EmreTech EmreTech changed the title Un-stringize type annotation passed into Option (fix for #513) Option Redesign (fix for #513) Apr 4, 2022
@Lulalaby
Copy link
Member

Closing in favor of #1251.
Core team decision.

@Lulalaby Lulalaby closed this Apr 11, 2022
v2.1 automation moved this from In progress to Done Apr 11, 2022
@EmreTech
Copy link
Contributor Author

I'll do some code fixes and try to improve this PR

@Lulalaby
Copy link
Member

We reviewed this pr internally and its too messy rn and wasnt in progress for a week / longer.
Clone will complete this within the next days.

Sorry, but this is the end decision for now.

@Pycord-Development Pycord-Development locked and limited conversation to collaborators Apr 11, 2022
@Lulalaby
Copy link
Member

2 months is just too long

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working priority: low Low Priority status: awaiting review Awaiting review from a maintainer
Projects
No open projects
v2.1
  
Done
Development

Successfully merging this pull request may close these issues.

Slash command option types are broken by stringized annotations