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

Add Resolver type, use it as a return type for field and fix tests #3383

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

Conversation

lukaspiatkowski
Copy link

Description

Change one of the overrides of strawberry.field to return -> Resolver[T] if resolver = ... is provided. This forces you to annotate your fields as field: Resolver[int] = ... and since Resolver is an abstract final class you cannot create an instance of it. This will prevent you from mistakenly writing or reading that field.

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 Feb 15, 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 Lukasz Piatkowski for the PR 👏

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

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.36%. Comparing base (e0eb336) to head (1560cd3).
Report is 2 commits behind head on main.

Current head 1560cd3 differs from pull request most recent head 8802c75

Please upload reports for the commit 8802c75 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3383      +/-   ##
==========================================
- Coverage   96.55%   95.36%   -1.20%     
==========================================
  Files         523      498      -25     
  Lines       33357    31148    -2209     
  Branches     5521     3809    -1712     
==========================================
- Hits        32208    29703    -2505     
- Misses        914     1236     +322     
+ Partials      235      209      -26     

@patrick91
Copy link
Member

couldn't we use Callable instead of Resolver? 😊

@lukaspiatkowski
Copy link
Author

I will fill up the documentation and release notes as soon as someone tells me there is a chance for merging this PR in :) Also my approach to writing the Resolver class itself is a bit hacky. I want to maintain my own fork with this change if it doesn't get merged, so I wanted to avoid changing core schema converting functions. I am fine fixing this part.

Copy link

codspeed-hq bot commented Feb 15, 2024

CodSpeed Performance Report

Merging #3383 will not alter performance

Comparing wavemm:lukas/resolver_type (1560cd3) with main (be9a2d5)

Summary

✅ 12 untouched benchmarks

@lukaspiatkowski
Copy link
Author

couldn't we use Callable instead of Resolver? 😊

Hmm, yes now that you say that we probably could do it, although it would be a bit of pain. Note that Callable is parametrized over 2 generic (Callable[Param, Ret]), in this case we don't care about Param part. But AFAIK Python doesn't let you yet write type aliases that parametrize part of the Generic, like so:

Resolver: TypeAlias = Callable[Any, T]

    field: Resolver[int] = strawberry.field(resolver=...)

So you would have to write the whole thing every time:

    field: Callable[Any, Int] = strawberry.field(resolver=...)`

which is IMO a mouthfull and not necessary correct - people could try and call this function, but without proper typing they wouldn't get the arguments right.

@lukaspiatkowski
Copy link
Author

Oh, I see that you are using libcst for codemoding backward incompatible changes. I was about to write one for myself, but I can add it here in this PR. Stay tuned! :)

@lukaspiatkowski
Copy link
Author

Codemod added as promised

@lukaspiatkowski lukaspiatkowski force-pushed the lukas/resolver_type branch 3 times, most recently from 42c21a7 to 4bd3bca Compare February 15, 2024 13:11
@lukaspiatkowski
Copy link
Author

One thing that I've noticed when using this new type on my codebase was that if you have an interface it might get difficult to get the types right. Consider this code:

@interface
class Base:
    a: int

@type
class A(Base):
    a: int = field(resolver=...)

@type
class B(Base):
    a: int

With the new Resolver type you won't be able to write:

@interface
class Base:
    a: int

@type
class A(Base):
    a: Resolver[int] = field(resolver=...)

@type
class B(Base):
    a: int

because the child class A changes the type of the base class Base. And for good reason! You wouldn't want this code to work without warnings:

def foo(base: Base) -> int:
    return base.a

def bar(a: A) -> int:
    return foo(a)

So for future reference if you stumble on this problem while changing your codebase my suggestion would be to write something like this:

@interface
class Base:
    a: Resolver[int]

@type
class A(Base):
    a: Resolver[int] = field(resolver=...)

@type
class B(Base):
	_a: Private[int]
    a: Resolver[int] = field(resolver=lambda self: self._a)

@lukaspiatkowski
Copy link
Author

@patrick91 any chance this is going to be merged or closed without merging?

@patrick91
Copy link
Member

@lukaspiatkowski not sure, sorry I didn't have time to take a proper look at this, I have set some time to look at PRs today, hopefully I get to this one

@lukaspiatkowski
Copy link
Author

Just friendly pinging @patrick91 here :)

@patrick91
Copy link
Member

@lukaspiatkowski sorry, still haven't had the time to properly think about this, I'll add it to my things for next week 😊

@lukaspiatkowski
Copy link
Author

@patrick91 friendly ping :)

@erikwrede
Copy link
Member

erikwrede commented May 28, 2024

Hey, I just went through your PR but I'm not fully into the details yet. Couldn't we also solve this with @overload, respecting wether a resolver was passed or not, as well? This would remove the need for additional resolver types or "Faking" object definitions. Need to try this out soon, but feel free to lmk what you think 😊

@lukaspiatkowski
Copy link
Author

@erikwrede not sure what you are proposing as this solution is changing the overload case for when the resolver is passed. Introducing the Resolver type is the core of this design, so removing the need for it would be missing the point here :)
The "Fake" part isn't pretty though, I agree. It was the fastest non-intrusive way of handling the addition of Resolver type (I had a feeling that you won't be merging this PR soon, so I kept it simple to be able to easily maintain my fork). Probably adding handling of Resolver somewhere deeper in the code would look better.

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.

4 participants