-
-
Notifications
You must be signed in to change notification settings - Fork 534
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
ANN401: Replace Any #3526
base: main
Are you sure you want to change the base?
ANN401: Replace Any #3526
Conversation
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature. Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) Here's the tweet text:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @brunodantas - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
strawberry/annotation.py
Outdated
try: | ||
return evaled_type._enum_definition | ||
except AttributeError: | ||
raise NotAStrawberryEnumError(evaled_type) | ||
|
||
def create_list(self, evaled_type: Any) -> StrawberryList: | ||
def create_list(self, evaled_type: TypeVar) -> StrawberryList: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Clarify the expected type for evaled_type
.
Using TypeVar
for evaled_type
might be too generic. If evaled_type
is expected to be a specific type or a set of types, consider using a more specific type hint or adding a type check.
@@ -139,7 +141,7 @@ def __init__( | |||
graphql_ide: Optional[GraphQL_IDE] = "graphiql", | |||
allow_queries_via_get: bool = True, | |||
subscriptions_enabled: bool = False, | |||
**kwargs: Any, | |||
**kwargs: Dict[Any, Any], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider using Mapping
instead of Dict
for kwargs.
Using Mapping
instead of Dict
for **kwargs
can provide more flexibility and better type safety, as Mapping
is a more general type.
**kwargs: Dict[Any, Any], | |
**kwargs: Mapping[Any, Any], |
next_: SyncExtensionResolver, | ||
source: Any, | ||
info: Info, | ||
**kwargs: Dict[Any, Any], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider using Mapping
instead of Dict
for kwargs.
Using Mapping
instead of Dict
for **kwargs
can provide more flexibility and better type safety, as Mapping
is a more general type.
**kwargs: Dict[Any, Any], | |
**kwargs: Mapping[Any, Any], |
@@ -69,7 +69,7 @@ def get_generic_alias(type_: Type) -> Type: | |||
raise AssertionError(f"No GenericAlias available for {type_}") # pragma: no cover | |||
|
|||
|
|||
def is_generic_alias(type_: Any) -> TypeGuard[_GenericAlias]: | |||
def is_generic_alias(type_: TypeVar) -> TypeGuard[_GenericAlias]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Clarify the expected type for type_
.
Using TypeVar
for type_
might be too generic. If type_
is expected to be a specific type or a set of types, consider using a more specific type hint or adding a type check.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3526 +/- ##
=======================================
Coverage 96.55% 96.56%
=======================================
Files 523 523
Lines 33357 33429 +72
Branches 5521 5540 +19
=======================================
+ Hits 32208 32280 +72
Misses 914 914
Partials 235 235 |
CodSpeed Performance ReportMerging #3526 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this, really appreciated 😊
I think the types might be wrong in a few places, can you double check?
@@ -21,7 +22,7 @@ def __init__( | |||
debug: bool, | |||
connection_init_wait_timeout: timedelta, | |||
get_context: Callable[..., Dict[str, Any]], | |||
get_root_value: Any, | |||
get_root_value: Callable[..., Optional[RootValue]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmh, I think we don't need Optional here 🤔 (same below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added Optional because of this implementation, is it wrong?
strawberry/strawberry/channels/handlers/ws_handler.py
Lines 118 to 119 in 2f02b49
async def get_root_value(self, request: ChannelsConsumer) -> Optional[RootValue]: | |
return None |
strawberry/annotation.py
Outdated
try: | ||
return evaled_type._enum_definition | ||
except AttributeError: | ||
raise NotAStrawberryEnumError(evaled_type) | ||
|
||
def create_list(self, evaled_type: Any) -> StrawberryList: | ||
def create_list(self, evaled_type: TypeVar) -> StrawberryList: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure these are typevars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ops, I double checked and most seem to be type
: a31f23d
@@ -24,11 +24,11 @@ class StrawberryAutoMeta(type): | |||
|
|||
""" | |||
|
|||
def __init__(self, *args: str, **kwargs: Any) -> None: | |||
def __init__(self, *args: str, **kwargs: Dict[Any, Any]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kwargs are implicitly marked as dicts 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm should we keep the Dict annotations? Ruff complained about the Any.
Description
Replacing a portion of the
Any
types with more specific types. This is my first code contribution, so I made it relatively short to make sure I'm on the right path. Does this require a RELEASE.md?Types of Changes
Issues Fixed or Closed by This PR
Checklist