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

Rewrite options #1251

Merged
merged 12 commits into from May 5, 2022
Merged

Rewrite options #1251

merged 12 commits into from May 5, 2022

Conversation

Dorukyum
Copy link
Member

@Dorukyum Dorukyum commented Apr 11, 2022

Summary

This modifies methods of slash commands related to option parsing and validation in order to optimize them and fix as many issues as possible.
Also allows a new design for creating options:

async def command(
    ctx,
    parameter: type = Option(**other_values),
)

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

@Dorukyum Dorukyum self-assigned this Apr 11, 2022
@Dorukyum Dorukyum added bug Something isn't working priority: high High Priority status: in progress Work in Progess feature Implements a feature Merge with squash labels Apr 11, 2022
@EmreTech

This comment was marked as off-topic.

@Lulalaby

This comment was marked as off-topic.

@EmreTech

This comment was marked as off-topic.

@Dorukyum

This comment was marked as off-topic.

@EmreTech

This comment was marked as off-topic.

@Lulalaby

This comment was marked as off-topic.

@Lulalaby Lulalaby mentioned this pull request Apr 11, 2022
10 tasks
@Lulalaby
Copy link
Member

Thank you clone ❤️

BobDotCom
BobDotCom previously approved these changes Apr 12, 2022
Lulalaby
Lulalaby previously approved these changes Apr 12, 2022
@krittick krittick added this to In progress in v2.0 via automation Apr 17, 2022
@krittick krittick added this to the v2.0 milestone Apr 17, 2022
@Dorukyum
Copy link
Member Author

Dorukyum commented Apr 18, 2022

Status: trying to solve a bug caused by the value of the cog atrribute of ext.bridge commands changing
Update: fixed

@Dorukyum Dorukyum requested review from BobDotCom, krittick, ChickenDevs and Middledot and removed request for ChickenDevs April 19, 2022 13:42
@krittick
Copy link
Contributor

Getting an error when using either of the following two methods to define an option:

@slash_command(name="opt_test")
async def opt_test(ctx: discord.ApplicationContext, test_option: Option(str, "Test")):
    await ctx.respond("test")

@slash_command(name="opt_test")
async def opt_test(ctx: discord.ApplicationContext, test_option: str = Option(description="Test")):
    await ctx.respond("test")
Traceback (most recent call last):
  File "C:\Users\ben\PycharmProjects\BasicBot\bot.py", line 48, in <module>
    async def opt_test(ctx: discord.ApplicationContext, test_option: Option(str, "Test")):
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\core.py", line 1560, in decorator
    return cls(func, **attrs)
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\core.py", line 637, in __init__
    self.options: List[Option] = self._parse_options(params)
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\core.py", line 690, in _parse_options
    option = Option(option, "No description provided")
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\options.py", line 133, in __init__
    _type = SlashCommandOptionType.from_datatype(input_type)
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\enums.py", line 670, in from_datatype
    if datatype.__name__ in ["Member", "User"]:
AttributeError: 'str' object has no attribute '__name__'. Did you mean: '__ne__'?

@codecov
Copy link

codecov bot commented Apr 30, 2022

Codecov Report

Merging #1251 (80fafa0) into master (217da10) will decrease coverage by 0.00%.
The diff coverage is 10.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1251      +/-   ##
==========================================
- Coverage   33.42%   33.42%   -0.01%     
==========================================
  Files          93       93              
  Lines       17615    17621       +6     
==========================================
+ Hits         5888     5890       +2     
- Misses      11727    11731       +4     
Flag Coverage Δ
pytest 33.42% <10.52%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
discord/enums.py 78.14% <0.00%> (-0.31%) ⬇️
discord/commands/core.py 18.00% <10.00%> (+0.24%) ⬆️
discord/commands/options.py 16.49% <20.00%> (-0.53%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 217da10...80fafa0. Read the comment docs.

@EmreTech
Copy link
Contributor

EmreTech commented May 5, 2022

Getting an error when using either of the following two methods to define an option:

@slash_command(name="opt_test")
async def opt_test(ctx: discord.ApplicationContext, test_option: Option(str, "Test")):
    await ctx.respond("test")

@slash_command(name="opt_test")
async def opt_test(ctx: discord.ApplicationContext, test_option: str = Option(description="Test")):
    await ctx.respond("test")
Traceback (most recent call last):
  File "C:\Users\ben\PycharmProjects\BasicBot\bot.py", line 48, in <module>
    async def opt_test(ctx: discord.ApplicationContext, test_option: Option(str, "Test")):
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\core.py", line 1560, in decorator
    return cls(func, **attrs)
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\core.py", line 637, in __init__
    self.options: List[Option] = self._parse_options(params)
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\core.py", line 690, in _parse_options
    option = Option(option, "No description provided")
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\options.py", line 133, in __init__
    _type = SlashCommandOptionType.from_datatype(input_type)
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\enums.py", line 670, in from_datatype
    if datatype.__name__ in ["Member", "User"]:
AttributeError: 'str' object has no attribute '__name__'. Did you mean: '__ne__'?

Are you including from __future__ import annotations?

@Lulalaby Lulalaby enabled auto-merge (squash) May 5, 2022 17:47
Copy link
Contributor

@EmreTech EmreTech left a comment

Choose a reason for hiding this comment

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

Looks good, I think someone should test it with the options decorator.

@krittick
Copy link
Contributor

krittick commented May 5, 2022

Getting an error when using either of the following two methods to define an option:

@slash_command(name="opt_test")
async def opt_test(ctx: discord.ApplicationContext, test_option: Option(str, "Test")):
    await ctx.respond("test")

@slash_command(name="opt_test")
async def opt_test(ctx: discord.ApplicationContext, test_option: str = Option(description="Test")):
    await ctx.respond("test")
Traceback (most recent call last):
  File "C:\Users\ben\PycharmProjects\BasicBot\bot.py", line 48, in <module>
    async def opt_test(ctx: discord.ApplicationContext, test_option: Option(str, "Test")):
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\core.py", line 1560, in decorator
    return cls(func, **attrs)
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\core.py", line 637, in __init__
    self.options: List[Option] = self._parse_options(params)
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\core.py", line 690, in _parse_options
    option = Option(option, "No description provided")
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\commands\options.py", line 133, in __init__
    _type = SlashCommandOptionType.from_datatype(input_type)
  File "C:\Users\ben\PycharmProjects\BasicBot\venv\lib\site-packages\discord\enums.py", line 670, in from_datatype
    if datatype.__name__ in ["Member", "User"]:
AttributeError: 'str' object has no attribute '__name__'. Did you mean: '__ne__'?

Are you including from __future__ import annotations?

Oh, whoops. Removing that resolves the issue.

@krittick
Copy link
Contributor

krittick commented May 5, 2022

Decorator works great, Union[TextChannel, VoiceChannel] and all the other good stuff work as well.

@krittick krittick requested a review from Lulalaby May 5, 2022 21:18
@Lulalaby Lulalaby disabled auto-merge May 5, 2022 21:20
@Lulalaby Lulalaby enabled auto-merge (squash) May 5, 2022 21:20
@Lulalaby Lulalaby merged commit 6a21ede into master May 5, 2022
Copy link
Member

@Lulalaby Lulalaby left a comment

Choose a reason for hiding this comment

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

@krittick its fucking 11pm xd
But here we go
Yeet

v2.0 automation moved this from In progress to Done May 5, 2022
@Lulalaby Lulalaby deleted the options-rewrite branch May 5, 2022 21:21
@krittick krittick restored the options-rewrite branch May 5, 2022 21:55
@martinbndr

This comment was marked as off-topic.

@jab416171
Copy link
Contributor

@martinbndr please create a new issue here: https://github.com/Pycord-Development/pycord/issues/new/choose

@Lulalaby Lulalaby deleted the options-rewrite branch June 9, 2022 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature Implements a feature priority: high High Priority status: in progress Work in Progess
Projects
Status: No status
v2.0
  
Done
Development

Successfully merging this pull request may close these issues.

Slash command option types are broken by stringized annotations
8 participants