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

Follow up to compliant Int work #3430

Merged
merged 8 commits into from
Dec 10, 2024

Conversation

phughes-scwx
Copy link
Contributor

@phughes-scwx phughes-scwx commented Dec 10, 2024

As requested, this is a follow up to #3409.

The introduction of the UUID binding in to the gqlgen init template was part of aligning this content and the version in the config docs. Since there is already a divergence in the federation section, I think it'd make sense to also remove the UUID from init template, since:

  • as @StevenACoffman stated, there is constant, tedious disagreement over which library to use for parsing UUIDs in Go and that should not be something this codebase has to re-litigate; IMO it is best, therefore, to de-emphasize the built in option for parsing UUIDs;
  • also, IMO, doing UUID parsing in the unmarshaler is an anti-pattern: scalar marshaler/unmarshalers should be focused on simple issues of converting from wire formats to basic data types, and avoid, where possible, validating or parsing to more complex types that could be better handled and communicated about in the resolver. This also leads me to think that the presence of a UUID unmarshaler tied into this codebase should be de-emphasized and eventually deprecated.

To that end, this PR removes that line that was added to the template. Ed: reverted.

Additionally, this PR ensures that uint32 unmarshalers return errors if the values they marshal overflow. It also lumps int32, uint32, and uint negative signed numbers into a group of errors that can be "caught" and handled as a class, either via logs/metrics or helpful messages to clients. This can be useful since an API designed to accept only certain types of integers may drift in scope and start to receive other integers that are valid in theory, but cause confusing errors.

I have:

  • Added tests covering the bug / feature (see testing)

@phughes-scwx phughes-scwx changed the title Follow up to Int Follow up to compliant Int work Dec 10, 2024
@coveralls
Copy link

coveralls commented Dec 10, 2024

Coverage Status

coverage: 73.919% (+0.5%) from 73.433%
when pulling 97cb976 on phughes-scwx:issue-3214-follow-up
into 26220f6 on 99designs:master.

@phughes-scwx phughes-scwx marked this pull request as ready for review December 10, 2024 18:17
@phughes-scwx
Copy link
Contributor Author

phughes-scwx commented Dec 10, 2024

Please do not merge even if you like: I see from coveralls some new, uncovered lines I'd like to add tests for. OBE

@phughes-scwx
Copy link
Contributor Author

OK, coverage updated and the uuid binding is retained in the init template. 🚀

@StevenACoffman
Copy link
Collaborator

This is great! I super appreciate this.

@StevenACoffman StevenACoffman merged commit 07c67c1 into 99designs:master Dec 10, 2024
17 checks passed
@phughes-scwx phughes-scwx mentioned this pull request Dec 10, 2024
@phughes-scwx phughes-scwx deleted the issue-3214-follow-up branch December 11, 2024 20:39
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.

3 participants