Refactor the backend typing system #906

BryceBeagle · 2021-05-04T15:31:06Z

This PR is a refactor of the typing system within the library. Previously, typing was handled individually by fields, arguments, and types with a hodgepodge of functions to tie it together. This branch creates a unified typing system that the object, fields, and arguments each hook into.

This PR mainly replaces the attributes that were stored on StrawberryArgument and StrawberryField with a hierarchy of StrawberryTypes.

There is more work to be done in the future, especially related to unifying the system by removing .type_definition and ._enum_definition monkeypatching.

There is also some future discussion to be had on this PR's implementation of StrawberryType.__eq__, but current thoughts are to look back into it after this PR is merged.

See #1063 for testing additions. This was done to prevent this PR from getting too long. It is also a good place to look to see how the changes here work.

To Do

~~- [ ] Tests involving StrawberryType.__eq__~~
~~- [ ] Think longer about how to implement StrawberryType.__hash__. Currently breaking the hash contract~~
~~- [ ] New unit tests for the StrawberryType types~~
(@patrick91 and I decided this should be addressed in another discussion/PR after this one is merged)

Types of Changes

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

…StrawberryType

# Conflicts: # strawberry/field.py

botberry · 2021-07-05T18:29:42Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

Refactor of the library's typing internals. Previously, typing was handled
individually by fields, arguments, and objects with a hodgepodge of functions to tie it
together. This change creates a unified typing system that the object, fields, and
arguments each hook into.

Mainly replaces the attributes that were stored on StrawberryArgument and
StrawberryField with a hierarchy of StrawberryTypes.

Introduces StrawberryAnnotation, as well as StrawberryType and some subclasses,
including StrawberryList, StrawberryOptional, and StrawberryTypeVar.

This is a breaking change if you were calling the constructor for StrawberryField,
StrawberryArgument, etc. and using arguments such as is_optional or child.

@strawberry.field no longer takes an argument called type_. It instead takes a
StrawberryAnnotation called type_annotation.

# Conflicts: # strawberry/field.py

patrick91

I'll test this on our work repo as soon as it is released. I tested it already, but I think we did some changes after that :)

strawberry/annotation.py

jkimbo

Nice work! Added some comments

strawberry/annotation.py

jkimbo · 2021-07-19T06:28:00Z

strawberry/arguments.py

                    raise MultipleStrawberryArgumentsError(
-                        field_name=origin.__name__, argument_name=python_name
+                        # TODO: This isn't the field name, it is the argument name
+                        field_name=self.python_name,


Do we no longer have access to the field at this point? It makes the error message nicer if we can say which field has the issue.

Yeah, we no longer have access to the field name here. I would agree it makes the error better, but I don't have a good idea of how to do it.

I believe that @patrick91 and I agreed to just include this regression (I should open an issue for it, thanks for the reminder), but I'm open to suggestions.

yes :) I think we can change a bit how we handle these errors, I'd love to have a better message and visualisation (for example pointing to the actual line), but that's something for the future :)

Ok in that case can you update the exception message to not include the bit about the field name: https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/exceptions.py#L121-L128

The test for it is now an xfail. I think we eventually want to go back to having the field name in the exception message. Should we still change it now?

I think we should change it because otherwise we have a misleading error message.

good point :D

Ok changed the error message for now

strawberry/exceptions.py

…into feature/strawberry_type_system

jkimbo

👍

BryceBeagle added 30 commits April 15, 2021 13:26

Move type.py to object_type.py to make room for the file for the new …

2607699

…StrawberryType

Rough implementation of StrawberryAnnotation/StrawberryType

7a594bd

More work, including a lot on the schema converter

d794eac

Tests get past collection

215cfff

"Fix" type annotation problems because of dataclasses

ea7874d

Fix issues with info object on fields

af759ac

Fix exception with unknown object type

b2a6d15

Deal with some type+annotation complexities

d27212b

Get argument_converter working

4dbecd5

Don't directly set argument type

9aa7078

Fix enum conversion

3f80246

Pass around EnumDefinitions instead of monkeypatched EnumMetas

4d58667

Harden _is_enum implementation for non-type objects

38f40d3

Fix StrawberryArgument instantiation

b4946ae

Fix directives

57b9253

Fix enum tests

470ffdf

Fix list tests

9a330d8

Fix union tests

dfd3845

Fix (most of) argument tests

b0c8692

Fix union tests

c56a314

Fix lambda resolvers

c4ac61d

Merge branch 'main' into feature/strawberry_type_system

a6e32aa

# Conflicts: # strawberry/field.py

Fix enum tests

a89be3d

Fix list type tests

24b10ac

Fix optional type tests

702224f

Fix string type tests

2e2bf2c

Better handling of (still broken) LazyType

3015c2e

Fix forward reference test

2d7b103

Fix pydantic tests

c7a6cfd

Fix more pydantic tests

dc5a482

patrick91 added 4 commits July 5, 2021 18:51

WIP

75a2d4c

Fix generics copy

de70661

Make comment clearer

f5d9f25

Lint

e0fd3b7

patrick91 self-requested a review July 5, 2021 18:29

patrick91 marked this pull request as ready for review July 5, 2021 18:29

patrick91 changed the title ~~(WIP) Refactor the backend typing system~~ Refactor the backend typing system Jul 5, 2021

Remove cast

825bc9b

patrick91 force-pushed the feature/strawberry_type_system branch from d70f3b5 to 825bc9b Compare July 5, 2021 18:37

Merge branch 'main' into feature/strawberry_type_system

6004626

# Conflicts: # strawberry/field.py

BryceBeagle mentioned this pull request Jul 15, 2021

Tests for StrawberryType refactor #1063

Merged

13 tasks

BryceBeagle added 3 commits July 15, 2021 11:02

Add __eq__ to StrawberryAnnotation

43a69fb

Add __eq__ (and __hash__) to StrawberryUnion

f17554b

Raise exception when trying to resolve StrawberryAnnotation(None)

9706d9b

patrick91 approved these changes Jul 18, 2021

View reviewed changes

strawberry/annotation.py Outdated Show resolved Hide resolved

BryceBeagle requested review from jkimbo and a team July 18, 2021 21:35

jkimbo suggested changes Jul 19, 2021

View reviewed changes

BryceBeagle added 6 commits July 19, 2021 08:44

Improve comment on Union

72deb42

Make MultipleStrawberryArgumentsError only take argument name for now

f62b09d

Tests for StrawberryType refactor (#1063)

4382b1e

Remove unused functions

4cdb284

Add a RELEASE.md

e3010e7

Merge remote-tracking branch 'origin/feature/strawberry_type_system' …

5b0221e

…into feature/strawberry_type_system

botberry added bot:has-release-file labels Jul 20, 2021

jkimbo approved these changes Jul 20, 2021

View reviewed changes

patrick91 merged commit 669ca12 into main Jul 20, 2021

patrick91 deleted the feature/strawberry_type_system branch July 20, 2021 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the backend typing system #906

Refactor the backend typing system #906

BryceBeagle commented May 4, 2021 •

edited

Loading

botberry commented Jul 5, 2021 •

edited

Loading

patrick91 left a comment

jkimbo left a comment

jkimbo Jul 19, 2021

BryceBeagle Jul 19, 2021

patrick91 Jul 19, 2021

jkimbo Jul 19, 2021

BryceBeagle Jul 19, 2021

jkimbo Jul 19, 2021

patrick91 Jul 19, 2021

BryceBeagle Jul 19, 2021

jkimbo left a comment

Refactor the backend typing system #906

Refactor the backend typing system #906

Conversation

BryceBeagle commented May 4, 2021 • edited Loading

To Do

Types of Changes

Issues Fixed or Closed by This PR

Checklist

botberry commented Jul 5, 2021 • edited Loading

patrick91 left a comment

Choose a reason for hiding this comment

jkimbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkimbo left a comment

Choose a reason for hiding this comment

BryceBeagle commented May 4, 2021 •

edited

Loading

botberry commented Jul 5, 2021 •

edited

Loading