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.

Sign up for GitHub

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

Jump to bottom

feat(app-cmds): channel, Annotated and Literal annotations #678

Merged
merged 45 commits into from Aug 8, 2022
Merged

feat(app-cmds): channel, Annotated and Literal annotations #678

merged 45 commits into from Aug 8, 2022

Conversation

alentoghostflame
Copy link
Collaborator

@alentoghostflame alentoghostflame commented Jun 13, 2022 •
edited by ooliver1

Summary

Closes #589, closes #591, closes #615 and fixes #673

Is a general rewrite of how annotations are handled inside of SlashCommandOption.

  • Optional
    • Handled a little better IMO, and works alongside everything else. Any Optional/None encountered will count the param as not being required (unless manually specified as required by SlashOption of course.)
  • Union
    • Handled alongside Optional, intended use is for the various *Channel classes.
  • Annotated
    • Handled properly, attempting to type it as the 2nd type given, going rightward if type checking fails and there are extras.
  • Literal

Due to annotations being touchy between Python 3.8 and 3.9/3.10, alongside this being a rewrite of how annotations are processed in slash, this needs thorough testing from a variety of people on those 3 versions of Python

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
    • I have run task pyright and fixed the relevant issues.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters). Kinda???
  • This PR is a breaking change (e.g. methods or parameters removed/renamed) Kinda???
    • had to change SlashCommandOption.converter to .converters as a list due to supporting multiple OptionConverters due to Union
  • This PR is not a code change (e.g. documentation, README, ...)

@alentoghostflame
Blagh
6920405 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@alentoghostflame
Actually added Literal support, made the *Channel classes properly ov…
a75ff2c 
…erride the channel types

Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@netlify
Copy link

netlify bot commented Jun 13, 2022 •
edited

✅ Deploy Preview for nextcord-gh-action ready!

Name Link
🔨 Latest commit 4a986dc
🔍 Latest deploy log https://app.netlify.com/sites/nextcord-gh-action/deploys/62e315a767ca29000958d131
😎 Deploy Preview https://deploy-preview-678--nextcord-gh-action.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@alentoghostflame
Copy link
Collaborator Author

Alright, small roadblock: Seems like Python 3.8 doesn't actually have typing.Annotated?

@DenverCoder1
Copy link
Collaborator

DenverCoder1 commented Jun 13, 2022 •
edited

Alright, small roadblock: Seems like Python 3.8 doesn't actually have typing.Annotated?

It's backported to 3.7 in the typing_extensions module

https://github.com/python/typing_extensions

@MaskDuck

This comment has been minimized.

Sign in to view
@ooliver1 ooliver1 changed the title Application Command Annotations feat(app-cmds): channel, Annotated and Literal annotations Jun 13, 2022
@ooliver1 ooliver1 added p: medium Priority: medium - should be worked on in the near future s: awaiting review Status: the issue or PR is awaiting reviews 2.1 The issue/PR should go for the 2.1 release labels Jun 13, 2022
@ooliver1 ooliver1 added this to the 2.1 milestone Jun 13, 2022
alentoghostflame and others added 10 commits June 13, 2022 18:45
@alentoghostflame
Switched from typing.Annotated to typing_extensions.Annotated
7e7008e 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@pre-commit-ci
style: auto fixes from pre-commit hooks
86d664a
@alentoghostflame
Added typing_extensions as a new hard requirement, added fix to make …
158d8af 
…pyright not upset

Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@alentoghostflame
Adding typing_extensions to docs requirements, little less hardcoding…
84ca030 
… for the docs github workflow

Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@alentoghostflame
Pyright, my IDE doesn't point out any of this, calm down.
99ea030 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@alentoghostflame
It's optional now, are you happy pyright?
ba8549c 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@pre-commit-ci
style: auto fixes from pre-commit hooks
2d59a23
@alentoghostflame
Please.
89060f6 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@alentoghostflame
At least you tell me what to do this time.
f0d25fb 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@alentoghostflame
Failure.
2f2e6cf 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
Skelmis
Skelmis previously requested changes Jun 19, 2022
View reviewed changes
nextcord/application_command.py Outdated Show resolved Hide resolved
@Skelmis
Copy link
Collaborator

Skelmis commented Jun 19, 2022

(Wrong button, meant to simply comment)

However, otherwise looks good assuming it is tested.

@ooliver1 ooliver1 self-requested a review June 23, 2022 12:17
ooliver1
ooliver1 requested changes Jun 23, 2022
View reviewed changes
nextcord/application_command.py Outdated Show resolved Hide resolved
nextcord/application_command.py Outdated Show resolved Hide resolved
nextcord/application_command.py Outdated Show resolved Hide resolved
@ooliver1 ooliver1 mentioned this pull request Jun 23, 2022
Merged
7 tasks
ooliver1
ooliver1 requested changes Jun 29, 2022
View reviewed changes
nextcord/application_command.py Outdated Show resolved Hide resolved
@alentoghostflame
docs(app cmds) fixes a word
41d0807 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@EmreTech

This comment was marked as resolved.

Sign in to view
@DenverCoder1 DenverCoder1 mentioned this pull request Jul 20, 2022
Closed
3 tasks
alentoghostflame and others added 3 commits July 22, 2022 19:54
@alentoghostflame
Fixed basic 3.8 support, needs more testing.
562025b 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@pre-commit-ci
style: auto fixes from pre-commit hooks
fbc9b5c
@alentoghostflame
format(app cmds): removes unnecessary critical logging, fixes line le…
a65ac4c 
…ngth of exception.

Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
EmreTech
EmreTech reviewed Jul 25, 2022
View reviewed changes
docs/api.rst Outdated Show resolved Hide resolved
nextcord/application_command.py Outdated Show resolved Hide resolved
nextcord/application_command.py Outdated Show resolved Hide resolved
nextcord/application_command.py Show resolved Hide resolved
ooliver1
ooliver1 requested changes Jul 26, 2022
View reviewed changes
docs/api.rst Outdated Show resolved Hide resolved
alentoghostflame and others added 3 commits July 26, 2022 10:04
@alentoghostflame @ooliver1
docs(app cmds) fix unnecessary line split.
3889cb2 
Co-authored-by: Oliver Wilkes <oliverwilkes2006@icloud.com>
@alentoghostflame
docs(app cmds) cleans up a bit, adds docstring for unpack_annotated
dff3833 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@alentoghostflame
Merge branch 'master' into app_cmd_annotations
4a986dc
ooliver1
ooliver1 requested changes Jul 31, 2022
View reviewed changes
Copy link
Member

@ooliver1 ooliver1 left a comment

Choose a reason for hiding this comment

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

Would you mind adding forum channel support? Else it seems good to me code-wise.

nextcord/application_command.py Outdated Show resolved Hide resolved
nextcord/application_command.py Outdated Show resolved Hide resolved
ooliver1
ooliver1 requested changes Jul 31, 2022
View reviewed changes
Copy link
Member

@ooliver1 ooliver1 left a comment •
edited

Choose a reason for hiding this comment

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

Would you mind adding forum channel support? Else it seems good to me code-wise.
network issue, I cannot seem to delete this comment

ooliver1
ooliver1 requested changes Aug 1, 2022
View reviewed changes
Copy link
Member

@ooliver1 ooliver1 left a comment

Choose a reason for hiding this comment

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

Also as docs/requirements.txt cannot be used in setup.py as docs/* is not on PyPI, setup.py's [docs] extra needs to be updated with the new dependency.

@ooliver1 ooliver1 added the t: enhancement Type: enhancement - new feature or request label Aug 3, 2022
alentoghostflame and others added 6 commits August 6, 2022 21:37
@alentoghostflame
feat(app cmds) add :class: support to slash command typehinting.
a02428b 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@pre-commit-ci
style: auto fixes from pre-commit hooks
fe1e046
@alentoghostflame
refactor(app cmds) removes an unneeded 'pass'
fce970c 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@alentoghostflame
fix(app cmds) raises a NotImplementedError if someone uses Literal in…
481a7ca 
… an undefined way

Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
@pre-commit-ci
style: auto fixes from pre-commit hooks
b9e3507
@alentoghostflame
docs(app cmds) add ForumChannel as a usable typehint to the docs
f3a0803 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
DenverCoder1
DenverCoder1 approved these changes Aug 7, 2022
View reviewed changes
nextcord/application_command.py Outdated Show resolved Hide resolved
@DenverCoder1
Copy link
Collaborator

Tested, everything seems to be working from what I can tell.

alentoghostflame and others added 2 commits August 7, 2022 18:33
@alentoghostflame @DenverCoder1
refactor(app cmds) Stops setting a variable when it doesn't need to b…
53d2f74 
…e set.

Co-authored-by: Jonah Lawrence <jonah@freshidea.com>
@alentoghostflame
docs: updates setup.py to match docs/requirements.txt
d7fcce5 
Signed-off-by: Alex Schoenhofen <alexanderschoenhofen@gmail.com>
ooliver1
ooliver1 approved these changes Aug 8, 2022
View reviewed changes
Copy link
Member

@ooliver1 ooliver1 left a comment

Choose a reason for hiding this comment

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

Failing pyright seems unrelated, LGTM

@ooliver1 ooliver1 merged commit 1df1bac into nextcord:master Aug 8, 2022
@alentoghostflame alentoghostflame deleted the app_cmd_annotations branch August 19, 2022 23:48
@ooliver1 ooliver1 removed the s: awaiting review Status: the issue or PR is awaiting reviews label Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Skelmis Skelmis Skelmis left review comments

@EmreTech EmreTech EmreTech left review comments

@DenverCoder1 DenverCoder1 DenverCoder1 approved these changes

@ooliver1 ooliver1 ooliver1 approved these changes

Assignees
No one assigned
Labels
2.1 The issue/PR should go for the 2.1 release p: medium Priority: medium - should be worked on in the near future t: enhancement Type: enhancement - new feature or request
Projects
No open projects
Nextcord progress
Status: Done
Milestone
2.1
6 participants
@alentoghostflame @DenverCoder1 @MaskDuck @Skelmis @EmreTech @ooliver1