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

Bug in JSONScalar example #1647

Merged
merged 13 commits into from
Feb 28, 2022
Merged

Bug in JSONScalar example #1647

merged 13 commits into from
Feb 28, 2022

Conversation

paulo-raca
Copy link
Contributor

@paulo-raca paulo-raca commented Feb 16, 2022

There was a bug in the JSONScalar example: parse_value takes a data structure (dicts/lists/primitives) instead of a JSON string, therefore json.loads is not necessary.

This PR started as a simple fix to the documentation, however @patrick91 suggested that we also added it into strawberry sources and tests, which I have done.

While at it, I did the same for the Base64 example -- Which I unfolded into Base16, Base32 and Base64

Finally, I also added support for the specified_by_url in scalars

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Checklist

  • My change requires a change to the documentation.
  • My code follows the code style of this project.
  • 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).

parse_value takes a data structure (dicts, lists and primitive types) instead of a JSON string
Also updated the description
@patrick91
Copy link
Member

good find! @paulo-raca would you mind adding a test for the JSON example? 😊

@paulo-raca
Copy link
Contributor Author

Sure!
Would it make sense to add JSONScalar into strawberry, or should it be kept only in the documentation/tests?

@patrick91
Copy link
Member

Sure! Would it make sense to add JSONScalar into strawberry, or should it be kept only in the documentation/tests?

I think we can add one!

Also see this comment here, the code is slightly different 🤔 #1409 (comment)

@paulo-raca
Copy link
Contributor Author

Also see this comment here, the code is slightly different thinking #1409 (comment)

The default implementation of parse_literal is self.parse_value(value_from_ast_untyped(value)), so that implementation is basically defining a broken parse_value and then skipping it. Hacky, but it does work 👍

@botberry
Copy link
Member

botberry commented Feb 16, 2022

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release adds the following scalar types:

  • JSON
  • Base16
  • Base32
  • Base64

they can be used like so:

from strawberry.scalar import Base16, Base32, Base64, JSON

@strawberry.type
class Example:
    a: Base16
    b: Base32
    c: Base64
    d: JSON

Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to Paulo Costa for the PR 👏

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

@paulo-raca paulo-raca changed the title Fix JSONScalar example Bug in JSONScalar example Feb 16, 2022
@paulo-raca
Copy link
Contributor Author

Sure! Would it make sense to add JSONScalar into strawberry, or should it be kept only in the documentation/tests?

I think we can add one!

I went ahead and did the same for the Base64 example -- Turns out the PR is now far from a one-line documentation fix

@github-actions
Copy link

Hi 👋 You can find a preview of the docs here:

https://strawberry.rocks/docs/pr/1647/types/scalars

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #1647 (1ed772f) into main (f25ac4b) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1647      +/-   ##
==========================================
- Coverage   98.16%   98.14%   -0.02%     
==========================================
  Files         129      129              
  Lines        4532     4540       +8     
  Branches      781      783       +2     
==========================================
+ Hits         4449     4456       +7     
  Misses         43       43              
- Partials       40       41       +1     

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 great! I've updated the release notes, I've removed the mention of the docs since they are only used for the changelog 😊

strawberry/scalars.py Show resolved Hide resolved
strawberry/schema/types/scalar.py Show resolved Hide resolved
Comment on lines +121 to +129
<Note>

The `Base16`, `Base32` and `Base64` scalar types are available in `strawberry.scalars`

```python
from strawberry.scalars import Base16, Base32, Base64
```

</Note>
Copy link
Member

Choose a reason for hiding this comment

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

shall we add an example/reason for when to use them?

RELEASE.md Outdated Show resolved Hide resolved
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.

Formatting change requests 👍

strawberry/scalars.py Outdated Show resolved Hide resolved
strawberry/scalars.py Outdated Show resolved Hide resolved
strawberry/scalars.py Outdated Show resolved Hide resolved
tests/schema/test_scalars.py Outdated Show resolved Hide resolved
@paulo-raca
Copy link
Contributor Author

paulo-raca commented Feb 28, 2022

Formatting change requests +1

All changes addressed 👍

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.

Thanks so much for this! 😊

@patrick91 patrick91 merged commit 133c9ce into strawberry-graphql:main Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants