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

rename _type_definition to __strawberry_definition__ #2836

Merged
merged 56 commits into from
Jun 20, 2023

Conversation

nrbnlulu
Copy link
Member

@nrbnlulu nrbnlulu commented Jun 11, 2023

Description

Rename _type_definition to __strawberry_definition__.

I understand that @BryceBeagle intended that StrawberryObject would be merged with TypeDefinition somehow in the future.
Currently I exposed WithStrawberryDefinition as a protocol that only includes __strawberry_definition__ in it. If in the future it would be a real type the annotations are already in place.

I am not very sure about the name __strawberry_object__ since it can be InputObject / Interface / ObjectType
and enums use _enum_definition, open to suggestions...
As discussed on discord every strawberry decorated type would have this dunder. Follow #2841 for more details.

TODO:

  • Add alias for _type_definition backward compat with warning.

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

@nrbnlulu nrbnlulu changed the title rename _type_definition to __strawberry_definition__ rename _type_definition to __strawberry_definition__ Jun 11, 2023
@codecov
Copy link

codecov bot commented Jun 11, 2023

Codecov Report

Merging #2836 (b3c2a94) into main (170f179) will increase coverage by 0.06%.
The diff coverage is 97.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2836      +/-   ##
==========================================
+ Coverage   96.30%   96.36%   +0.06%     
==========================================
  Files         206      207       +1     
  Lines        8911     8949      +38     
  Branches     1641     1644       +3     
==========================================
+ Hits         8582     8624      +42     
+ Misses        208      206       -2     
+ Partials      121      119       -2     

@nrbnlulu nrbnlulu marked this pull request as ready for review June 11, 2023 08:26
@botberry
Copy link
Member

botberry commented Jun 11, 2023

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release renames _type_definition to __strawberry_definition__. This doesn't change the public API of Strawberry, but if you were using _type_definition you can still access it, but it will be removed in future.


Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to ניר for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

@nrbnlulu nrbnlulu changed the title rename _type_definition to __strawberry_definition__ rename _type_definition to __strawberry_object__ Jun 11, 2023
strawberry/types/types.py Outdated Show resolved Hide resolved
@skilkis
Copy link
Contributor

skilkis commented Jun 12, 2023

Once again great work @nrbnlulu!! Just left two minor comments, the rest looks good to me 🚀

Copy link
Contributor

@bellini666 bellini666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an awesome change in general! :)

Made some minor suggestions

strawberry/exceptions/invalid_argument_type.py Outdated Show resolved Hide resolved
strawberry/type.py Outdated Show resolved Hide resolved
strawberry/types/types.py Outdated Show resolved Hide resolved
@nrbnlulu nrbnlulu requested a review from bellini666 June 14, 2023 10:44
strawberry/utils/deprecations.py Outdated Show resolved Hide resolved
tests/relay/test_utils.py Outdated Show resolved Hide resolved
tests/schema/test_directives.py Outdated Show resolved Hide resolved
tests/test_deprecations.py Outdated Show resolved Hide resolved
strawberry/codegen/query_codegen.py Outdated Show resolved Hide resolved
@@ -151,6 +152,12 @@ def _process_type(
_fields=fields,
is_type_of=is_type_of,
)
# TODO: remove when deprecating _type_definition
DeprecatedDescriptor(
DEPRECATION_MESSAGES._TYPE_DEFINITION,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler/better to just put the message in here directly, instead of defining and importing DEPRECATION_MESSAGES?

Copy link
Member Author

@nrbnlulu nrbnlulu Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it 3 times, would be duplicated ig.. it also easier to remove it afterwards I think.

@@ -340,7 +344,7 @@ def get_graphql_input_fields(
)

def from_input_object(self, object_type: type) -> GraphQLInputObjectType:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just notice that from_input_object is the only converter that receives a type instead of StrawberryObjectDefinition

@patrick91 should we try to normalize this? Don't know if in this PR, but since the breaking change is happening it would be nice. Either this function should receive the object definition, like the others (e.g. from_object, from_interface, etc), or those others should receive the type directly (even typing it as type: WithObjectDefinition)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's do in another PR 😊

nrbnlulu and others added 7 commits June 14, 2023 19:15
Co-authored-by: Thiago Bellini Ribeiro <hackedbellini@gmail.com>
Co-authored-by: Thiago Bellini Ribeiro <hackedbellini@gmail.com>
# Conflicts:
#	tests/schema/test_extensions.py
@nrbnlulu nrbnlulu requested a review from bellini666 June 18, 2023 17:35
strawberry/type.py Outdated Show resolved Hide resolved
nrbnlulu and others added 3 commits June 18, 2023 21:16
Co-authored-by: Thiago Bellini Ribeiro <hackedbellini@gmail.com>
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.

Thank you so much for doing this! <3

RELEASE.md Outdated Show resolved Hide resolved
@patrick91 patrick91 merged commit cd0ab64 into strawberry-graphql:main Jun 20, 2023
39 checks passed
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.

None yet

5 participants