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

Allow other formats when unmarshalling time #1964

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ItalyPaleAle
Copy link

This PR makes the method that unmarshalls times a bit more flexible.

Currently, only dates that are in full RFC339 format are supported, strictly. This adds a couple more formats relaxing the parser:

  • 2006-01-02 15:04:05.999999999 -> allows things like 2022-02-10 10:20:30 (nanoseconds are optional)
  • 2006-01-02 -> allowing things like 2022-02-10 (which defaults to midnight)

When a time zone is not specified, it defaults to UTC (as the default for time.Parse)

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@giautm
Copy link
Contributor

giautm commented Feb 14, 2022

I think you should mapping Time to your custom scalar instead.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 74.46% when pulling 9a0650b on ItalyPaleAle:unmarshal-time-format into d7da5b0 on 99designs:master.

@ItalyPaleAle
Copy link
Author

I think you should mapping Time to your custom scalar instead.

Certainly an option. I still thought of submitting this as it doesn't necessarily change the behavior of the un-marshaller but just adds some more flexibility, for example adding support for formats that work on Postgres and many other places.

Up to the maintainers if they want to consider this or not :)

@giautm
Copy link
Contributor

giautm commented Feb 14, 2022

This PR changes the format to accept date-only layout (2006-01-02) can break other applications that don't expect it.

PS: Time layout format was a very-old issue in the Golang repository. :(

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.

6 participants