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

ANN401: Replace Any #3526

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

brunodantas
Copy link
Contributor

@brunodantas brunodantas commented May 30, 2024

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

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@botberry
Copy link
Member

botberry commented May 30, 2024

Hi, thanks for contributing to Strawberry 🍓!

We noticed that this PR is missing a RELEASE.md file. We use that to automatically do releases here on GitHub and, most importantly, to PyPI!

So as soon as this PR is merged, a release will be made 🚀.

Here's an example of RELEASE.md:

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:

🆕 Release (next) is out! Thanks to Bruno Dantas for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

strawberry/annotation.py Show resolved Hide resolved
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:
Copy link
Contributor

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],
Copy link
Contributor

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.

Suggested change
**kwargs: Dict[Any, Any],
**kwargs: Mapping[Any, Any],

next_: SyncExtensionResolver,
source: Any,
info: Info,
**kwargs: Dict[Any, Any],
Copy link
Contributor

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.

Suggested change
**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]:
Copy link
Contributor

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.

Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.56%. Comparing base (e0eb336) to head (e147cf8).
Report is 9 commits behind head on main.

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           

Copy link

codspeed-hq bot commented May 30, 2024

CodSpeed Performance Report

Merging #3526 will not alter performance

Comparing brunodantas:fix-ann-401 (e147cf8) with main (cdf0278)

Summary

✅ 12 untouched benchmarks

Copy link
Member

@patrick91 patrick91 left a 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]],
Copy link
Member

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)

Copy link
Contributor Author

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?

async def get_root_value(self, request: ChannelsConsumer) -> Optional[RootValue]:
return None

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:
Copy link
Member

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?

Copy link
Contributor Author

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:
Copy link
Member

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 😊

Copy link
Contributor Author

@brunodantas brunodantas May 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants