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

Clean up StrawberryField and StrawberryArgument interface #916

Merged
merged 6 commits into from
May 28, 2021

Conversation

jkimbo
Copy link
Member

@jkimbo jkimbo commented May 6, 2021

Description

  • Change the signature of StrawberryField to make it easier to instantiate directly (defaulting arguments to None)
  • Rename default_value to default in StrawberryField
  • Rename default_value to default in StrawberryArgument
  • Remove usages of undefined in favour of UNSET (note: I haven't actually removed undefined but I think we can do that now)

These changes are pretty minor but they are the ground work for allowing custom fields. I was just going to do all the changes necessary but #906 changes some of the code I was going to touch so I'll write up my proposal for it first before doing the code changes.

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

@jkimbo jkimbo requested review from patrick91 and BryceBeagle May 6, 2021 11:48
@botberry
Copy link
Member

botberry commented May 6, 2021

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


  • Remove usages of undefined in favour of UNSET
  • Change the signature of StrawberryField to make it easier to instantiate
    directly. Also change default_value argument to default
  • Rename default_value to default in StrawberryArgument

@botberry

This comment has been minimized.

1 similar comment
@botberry

This comment has been minimized.

@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #916 (eef3359) into main (4fc9c9b) will not change coverage.
The diff coverage is 90.90%.

@@           Coverage Diff           @@
##             main     #916   +/-   ##
=======================================
  Coverage   97.41%   97.41%           
=======================================
  Files          76       76           
  Lines        2592     2592           
  Branches      362      362           
=======================================
  Hits         2525     2525           
  Misses         41       41           
  Partials       26       26           

Comment on lines +20 to +21
def __bool__(self):
return False
Copy link
Member

Choose a reason for hiding this comment

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

Advantage of adding truthiness?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved this class around. Not sure what the point of this is. @patrick91 do you know?

Copy link
Member

Choose a reason for hiding this comment

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

it's to make sure you can use it in if statements:

def something(a: int = UNSET):
    if a:
        return 

    ...

(note this check is silly but it's to demonstrate the idea :) )

Copy link
Member

Choose a reason for hiding this comment

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

That seems like it could lead to bugs. What if a is 0?

Copy link
Member

Choose a reason for hiding this comment

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

sorry I missed this comment! that's a good point 😊

but then that means you always need to check if it is unset before doing anything with it, right? I guess that's similar to using None, so I think we can remove it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW UNSET will be changing significantly with this change anyway: #872

strawberry/arguments.py Outdated Show resolved Hide resolved
strawberry/arguments.py Outdated Show resolved Hide resolved
strawberry/arguments.py Outdated Show resolved Hide resolved
strawberry/field.py Outdated Show resolved Hide resolved
strawberry/field.py Outdated Show resolved Hide resolved
strawberry/field.py Outdated Show resolved Hide resolved
strawberry/field.py Show resolved Hide resolved
@jkimbo jkimbo requested a review from BryceBeagle May 7, 2021 08:03
@jkimbo jkimbo force-pushed the strawberry-field-cleanup branch from c0bcf7c to eef3359 Compare May 16, 2021 10:05
@jkimbo
Copy link
Member Author

jkimbo commented May 16, 2021

@patrick91 @BryceBeagle I've added some release notes. I think this is good to go now.

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! There's a couple of comments, but we can merge the PR and deal with them later if you prefer @jkimbo 😊

Comment on lines +20 to +21
def __bool__(self):
return False
Copy link
Member

Choose a reason for hiding this comment

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

sorry I missed this comment! that's a good point 😊

but then that means you always need to check if it is unset before doing anything with it, right? I guess that's similar to using None, so I think we can remove it :)

strawberry/field.py Show resolved Hide resolved
@jkimbo jkimbo merged commit b4e5b25 into main May 28, 2021
@jkimbo jkimbo deleted the strawberry-field-cleanup branch May 28, 2021 08:28
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