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 custom dataclass init function #1187

Merged
merged 11 commits into from
Sep 5, 2022
Merged

Add custom dataclass init function #1187

merged 11 commits into from
Sep 5, 2022

Conversation

jkimbo
Copy link
Member

@jkimbo jkimbo commented Aug 30, 2021

Description

This PR adds a custom dataclass init function that enforces that all args get passed as keyword arguments. This allows us to avoid the problem where a type cannot define a field with a default value before a field that doesn't have a default value.

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 Aug 30, 2021

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release changes how dataclasses are created to make use of the new
kw_only argument in Python 3.10 so that fields without a default value can now
follow a field with a default value. This feature is also backported to all other
supported Python versions.

More info: https://docs.python.org/3/library/dataclasses.html#dataclasses.dataclass

For example:

# This no longer raises a TypeError

@strawberry.type
class MyType:
    a: str = "Hi"
    b: int

⚠️ This is a breaking change! Whenever instantiating a Strawberry type make sure
that you only pass values are keyword arguments:

# Before:

MyType("foo", 3)

# After:

MyType(a="foo", b=3)

Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to Jonathan Kim for the PR 👏

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

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #1187 (bbc9ebe) into main (431db0a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1187   +/-   ##
=======================================
  Coverage   98.19%   98.20%           
=======================================
  Files         160      162    +2     
  Lines        6388     6422   +34     
  Branches     1205     1216   +11     
=======================================
+ Hits         6273     6307   +34     
  Misses         58       58           
  Partials       57       57           

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.

This might be good for now, I wan't to see what @BryceBeagle thinks before merging though 😊

I think we should still vendorize in future :)

@jkimbo jkimbo self-assigned this Sep 1, 2021
Copy link
Member

@BryceBeagle BryceBeagle left a comment

Choose a reason for hiding this comment

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

I think this is a pretty good solution for now, just a few minor changes I see here

strawberry/utils/dataclasses.py Outdated Show resolved Hide resolved
strawberry/utils/dataclasses.py Outdated Show resolved Hide resolved
strawberry/utils/dataclasses.py Outdated Show resolved Hide resolved
strawberry/utils/dataclasses.py Outdated Show resolved Hide resolved
strawberry/utils/dataclasses.py Outdated Show resolved Hide resolved
@jkimbo
Copy link
Member Author

jkimbo commented Dec 18, 2021

@patrick91 @BryceBeagle this is ready for a review again. Sorry for the delay!

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.

Looks good to me, I think we need to make this release more visible somehow, to make sure the breaking change is seen by everyone 😊

Comment on lines +3 to +6
This release changes how dataclasses are created to make use of the new
`kw_only` argument in Python 3.10 so that fields without a default value can now
follow a field with a default value. This feature is also backported to all other
supported Python versions.
Copy link
Member

Choose a reason for hiding this comment

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

💯

@jkimbo
Copy link
Member Author

jkimbo commented Dec 20, 2021

@patrick91 yep I agree. Any suggestions on how to do that?

@patrick91
Copy link
Member

@patrick91 yep I agree. Any suggestions on how to do that?

Maybe we can send a newsletter for the end of the year and include it there! I can write something in the next few days ☺️

@patrick91
Copy link
Member

/pre-release

@botberry
Copy link
Member

Pre-release

👋

Releasing commit [50d7463] to PyPi as pre-release! 📦

@botberry
Copy link
Member

Pre-release

👋

Pre-release 0.94.0-dev.1642180413 [50d7463] has been released on PyPi! 🚀
You can try it by doing:

poetry add strawberry-graphql==0.94.0-dev.1642180413

@nrbnlulu nrbnlulu mentioned this pull request Jul 10, 2022
11 tasks
@jkimbo
Copy link
Member Author

jkimbo commented Sep 5, 2022

@patrick91 is this still relevant?

@patrick91
Copy link
Member

@patrick91 is this still relevant?

yes, I need to merge it! sorry about that, will rebase it tonight and see what we need to do before merging it :)

@patrick91 patrick91 force-pushed the custom-dataclass-init branch 2 times, most recently from 1d00daf to 8370a2a Compare September 5, 2022 21:09
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.

Hey, so sorry it took me so long to merge this! Thanks for the nudge and all the work @jkimbo 😊

@patrick91 patrick91 merged commit 1b14a45 into main Sep 5, 2022
@patrick91 patrick91 deleted the custom-dataclass-init branch September 5, 2022 21:26
@nrbnlulu nrbnlulu mentioned this pull request Sep 6, 2022
21 tasks
paulo-raca added a commit to paulo-raca/strawberry that referenced this pull request Sep 22, 2022
…e behaviour introduced in strawberry-graphql#1187

- Set `kw_only=True` in DataclassAttribute object
- Remove code to check that `Attributes without a default cannot follow attributes with one`
patrick91 added a commit that referenced this pull request Sep 22, 2022
* Update Mypy plugin to treat strawberry fields as kw_only, matching the behaviour introduced in #1187

- Set `kw_only=True` in DataclassAttribute object
- Remove code to check that `Attributes without a default cannot follow attributes with one`

* Fix type issue

* Update types

* Fix types

* Update pyright support

Co-authored-by: Patrick Arminio <patrick.arminio@gmail.com>
@jceresini
Copy link

This introduced a breaking change for us when upgrading from 0.119.1 to 0.131.1. We're using federation, and defined a type that is handled by another subgraph:

@strawberry.federation.type(extend=True, keys=["id"])
class Resource:
    id: strawberry.ID = strawberry.federation.field(external=True)

# [...]

@strawberry.federation.type(keys=["id"], name="Event" )
class Event:
    id: strawberry.ID
    resource_id: strawberry.Private[str]

    @strawberry.field
    def resource(self) -> Resource:
        return Resource(self.resource_id)

After an update we started seeing this error in our logs

        return Resource(self.resource_id)
TypeError: __init__() takes 1 positional argument but 2 were given

It was a simple fix (passing the id as a kwarg) but tracking it down was a bit frustrating, and unexpected for a minor release. Sharing this in case others run into similar issues.

@jceresini
Copy link

jceresini commented Sep 27, 2022

I tracked down this PR while addressing the issue in our code, and commented without reading the above. The PR clearly explains that its a breaking change. 🤦‍♂️ I'll pay more attention to release notes when approving changes that bump the strawberry version.

@jkimbo
Copy link
Member Author

jkimbo commented Sep 27, 2022

@jceresini apologies for the inconvenience. We try hard to not make breaking changes but sometimes it's unavoidable.

I think this is also an artifact of the quick release cycle that Strawberry has which often results in people upgrading between quite large version ranges and so it's hard to identify all the potential breaking changes. We might benefit from something on the website that allows you show the difference between versions and view all the changes at once. What do you think @patrick91 ?

@patrick91
Copy link
Member

@jceresini apologies for the inconvenience. We try hard to not make breaking changes but sometimes it's unavoidable.

I think this is also an artifact of the quick release cycle that Strawberry has which often results in people upgrading between quite large version ranges and so it's hard to identify all the potential breaking changes. We might benefit from something on the website that allows you show the difference between versions and view all the changes at once. What do you think @patrick91 ?

yes, I'd love to have something like that! were you thinking of a page with a list of breaking changes and pending deprecations? or something else entirely?

@jceresini sorry about this! Ideally we should have done this in a major release, but unfortunately we haven't made the jump to v1 just yet

@jkimbo
Copy link
Member Author

jkimbo commented Sep 27, 2022

yes, I'd love to have something like that! were you thinking of a page with a list of breaking changes and pending deprecations? or something else entirely?

Highlighting breaking changes would be ideal but I'm not sure how we would extract that. As a first step it would be great to just list all the titles of the changes and you can open each one to find out more. More like traditional release notes. I'm imagining something like this: https://react-native-community.github.io/upgrade-helper/

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.

5 participants