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

[lukaspiatkowski]

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

• Fields defined with ... = field(resolver=...) are treated like regular dataclass attributes by type checkers #3377

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]

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)

[codecov]

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]

couldn't we use Callable instead of Resolver? 😊

[lukaspiatkowski]

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.

[codspeed-hq]

CodSpeed Performance Report

Merging #3383 will not alter performance

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

Summary

✅ 12 untouched benchmarks

[lukaspiatkowski]

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]

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]

Codemod added as promised

[lukaspiatkowski]

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]

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

[patrick91]

@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]

Just friendly pinging @patrick91 here :)

[patrick91]

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

[lukaspiatkowski]

@patrick91 friendly ping :)

[erikwrede]

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]

@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.