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

Support JSON serialization and deserialization of atomic.Time #125

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

BRUHItsABunny
Copy link

@BRUHItsABunny BRUHItsABunny commented Nov 25, 2022

Before opening your pull request, please make sure that you've:

  • updated the changelog if the change is user-facing;
  • added tests to cover your changes;
  • run the test suite locally (make test); and finally,
  • run the linters locally (make lint).

Thanks for your contribution!

@CLAassistant
Copy link

CLAassistant commented Nov 25, 2022

CLA assistant check
All committers have signed the CLA.

@BRUHItsABunny
Copy link
Author

BRUHItsABunny commented Nov 25, 2022

Fixes #124

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

I think we should implement MarshalText instead, since the JSON encoding of a time is a string, and MarshalText will work in more cases, and matches atomic.String better.

@BRUHItsABunny
Copy link
Author

Agreed, that does make more sense.
Will make the change shortly.

@BRUHItsABunny
Copy link
Author

@prashantv My apologies for the delay, I've updated the PR with the latest comments in mind.

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