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 NodaTime scalar types #3795

Merged
merged 22 commits into from
Jun 9, 2021
Merged

Add NodaTime scalar types #3795

merged 22 commits into from
Jun 9, 2021

Conversation

benmccallum
Copy link
Collaborator

@benmccallum benmccallum commented Jun 4, 2021

Original repo:
https://github.com/shoooe/hotchocolate-nodatime

Tasks:

  • Bring over src + tests.
  • Better support for StrawberryShake via custom scalars
  • Validate conformity with existing codebase.
  • Approve new public API.
  • We have type name overlaps, but I've used the HotChocolate.Types.NodaTime namespace to keep them separate. Do we need to consider anything else? Can't imagine folks would want to use current + nodatime versions at once, but would they be able to with type renaming easily enough?
  • Type descriptions are nodatime type descs. That's ok, but it's nice in HC's ones how they refer to standards where appropriate so you know the format. Maybe we should do that + link to nodatime docs in the descs?
  • Think about docs. Currently I've brought over the original README.md
  • Think about crediting the original author. I've added a note at top of the readme for now.
  • Get package ownership in NuGet.
  • CI (including code coverage?)
  • CD.
  • ⚠️ ⚠️ Add shoooe as co-author in the merge commit so he gets cred there too.
  • Deprecate existing package versions on NuGet? Assuming we'll version this as 11.x, but as long as it supports 11.0 onwards that should be ok.
  • Deprecate existing repository (PR over there to point over here, then get shoe to archive it?)

@github-actions github-actions bot added 🌶️ hot chocolate 📚 documentation This issue is about working on our documentation. labels Jun 4, 2021
@benmccallum benmccallum changed the base branch from develop to main June 4, 2021 13:53
@benmccallum
Copy link
Collaborator Author

@tobias-tengler , what do you think about documenting this? In a similar way to how extended scalars was documented? We just tabulate the readme in here in the docs?

@tobias-tengler
Copy link
Collaborator

tobias-tengler commented Jun 4, 2021

@tobias-tengler , what do you think about documenting this? In a similar way to how extended scalars was documented? We just tabulate the readme in here in the docs?

Yes, I think that would be best for now.
Personally I think it would be better, if we could get it to integrate into the HotChocolate.Types.Scalars package. That would be the most seamless in my opinion (from a usage as well as documentation persective). We could call them something likeNodaDateTimeZone for example to avoid the name collisions.

@benmccallum
Copy link
Collaborator Author

I considered that too @tobias-tengler; the concern I had is that then we force someone to take on a dependency they might not want/need (NodaTime). That might be acceptable though.

@benmccallum
Copy link
Collaborator Author

@michaelstaib , one thing I'm wondering about is how to elegantly do the custom scalar ISerializer that StrawberryShake integration would benefit from.

It seems a shame to need to copy/link the code of the core scalar type's TryDeserialize implementation over in to another project like StrawberryShake.Types.NodaTime. It's a problem that I imagine we should solve for all the existing extended scalars too though.

Any thoughts on how we should approach it?

@benmccallum benmccallum changed the title Absorbs shoooe/hotchocolate-nodatime in to core repo Absorb shoooe/hotchocolate-nodatime Jun 4, 2021
@shoooe
Copy link
Contributor

shoooe commented Jun 8, 2021

I've added ChilliCream as package owner. 👍

@shoooe
Copy link
Contributor

shoooe commented Jun 8, 2021

I've also added a warning in the README of the old repository and archived it.

@tobias-tengler
Copy link
Collaborator

@benmccallum I will go over this more thoroughly tonight. I think we should have another look together tomorrow. We should add for strawberry shake a separate issue otherwise this will take to long. The StrawberryShake serializers can go into 11.4 or 12.

@tobias-tengler do we want to put docs into this PR or a separate one.

@shoooe what is your GitHub email that we can use for the commit?

I can convert the documentation in the README to the website real quick. I just hope it doesn't get to full ^^

@sonarcloud
Copy link

sonarcloud bot commented Jun 9, 2021

@michaelstaib michaelstaib changed the title Absorb shoooe/hotchocolate-nodatime Add NodaTime scalar types Jun 9, 2021
@michaelstaib
Copy link
Member

I think we are ready.

michaelstaib
michaelstaib previously approved these changes Jun 9, 2021
@michaelstaib
Copy link
Member

I ticked Better support for StrawberryShake via custom scalars since it is out of scope for this PR.... lets create an issue for that. We have a couple of other serializers to introduce and can do that together with that effort.

@michaelstaib
Copy link
Member

@benmccallum what is Approve new public schema. about?

michaelstaib
michaelstaib previously approved these changes Jun 9, 2021
@michaelstaib michaelstaib merged commit d7386c3 into main Jun 9, 2021
@michaelstaib michaelstaib deleted the benmc/types-nodatime branch June 9, 2021 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 documentation This issue is about working on our documentation. 🌶️ hot chocolate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants