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

refactor(ext.commands): completely rewrite Range and String, require type argument #991

Merged
merged 16 commits into from
Jun 20, 2023

Conversation

shiftinv
Copy link
Member

@shiftinv shiftinv commented Mar 29, 2023

Summary

(I recommend using the split diff for looking at the changes in this PR, the unified diff is fairly unreadable as-is.)

As of pyright 1.1.299, Range[1, 2] is no longer accepted; that old syntax was sort of abusing __class_getitem__ before, so this is an issue on our end.

This PR completely reworks Range and String, adding a ton of validation+tests and making them work with recent versions of pyright and mypy without misusing the type system c:
The new implementation is based on the general idea of aliasing these types to typing.Annotated at type-checking time.

The old Range[1, 2] syntax is deprecated, but still works (at runtime), for now; the new/proper way to use them is Range[int, 1, 2], Range[float, 1, 2], and String[str, 1, 2].
In the same vein, the mypy plugin that was added later and made these work is now a stub, raising a deprecation warning.

Unresolved Issues
  • I'm still undecided about validating float range bounds; unlike int bounds, which the API limits to 2**53 as expected, float bounds quietly lose precision past (-)2**53 on the API side. Should we emit warnings/errors when going above/below that?

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running pdm lint
    • I have type-checked the code by running pdm pyright
  • 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, ...)

@shiftinv shiftinv added p: medium Medium priority s: needs review Issue/PR is awaiting reviews t: deprecation Deprecation of existing features t: bugfix labels Mar 29, 2023
@shiftinv shiftinv added this to the disnake v2.9 milestone Mar 29, 2023
Copy link
Member

@onerandomusername onerandomusername left a comment

Choose a reason for hiding this comment

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

This ends up being a breaking change, yet isn't documented as such.

edit: nevermind, tested incorrectly.

@shiftinv shiftinv modified the milestone: disnake v2.9 Jun 16, 2023
@onerandomusername onerandomusername modified the milestone: disnake v2.9 Jun 18, 2023
Copy link
Member

@Sharp-Eyes Sharp-Eyes left a comment

Choose a reason for hiding this comment

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

Haven't tested, but implementation-wise, this looks good to me ^^

Copy link
Member

@onerandomusername onerandomusername left a comment

Choose a reason for hiding this comment

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

This is backwards compatible, but I'm not sure about the changes to String, why does String need the type argument?

disnake/ext/commands/params.py Show resolved Hide resolved
@Sharp-Eyes
Copy link
Member

Sharp-Eyes commented Jun 19, 2023

This is backwards compatible, but I'm not sure about the changes to String, why does String need the type argument?

Afaik that's just to make it work with typing.Annotated, which is what commands.String is during type checking. It's a bit unfortunate, but I don't think there's a way around it.

edit:
Note that typing.Annotated expects its first arg to be a type, and anything else to be the "annotations" we desire. I don't think there's a way to make that first arg default to str and still have the type able to take annotations.

@shiftinv
Copy link
Member Author

Note that typing.Annotated expects its first arg to be a type, and anything else to be the "annotations" we desire. I don't think there's a way to make that first arg default to str and still have the type able to take annotations.

exactly this - Annotated always requires the first argument to be a type (see https://peps.python.org/pep-0593/), we just have to work with that :/
Something we could consider doing instead is deprecating String and supporting Range[str, 1, 2] instead, but that looks a bit off as well.

@onerandomusername onerandomusername merged commit 051a963 into master Jun 20, 2023
@onerandomusername onerandomusername deleted the refactor/ext-range branch June 20, 2023 21:04
@shiftinv shiftinv removed the s: needs review Issue/PR is awaiting reviews label Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: medium Medium priority t: bugfix t: deprecation Deprecation of existing features
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants