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

feat: add timepb support library #60

Merged
merged 8 commits into from
Feb 8, 2022
Merged

feat: add timepb support library #60

merged 8 commits into from
Feb 8, 2022

Conversation

robert-zaremba
Copy link
Collaborator

Summary

Moved from: cosmos/cosmos-sdk#11085

A support library to do time operation with protobuf Timestamp

Motivation

With new proto directions we are going away from go stdlib time.Time to the protobuf Timestamppb package.
We often do time operations (eg comparing times) and we need a canonical library for Timestamppb rather then repeating / copying code.

Updates

  • @fdymylja suggested to put it in types/wellknown/time. We are not creating a new types, but a support library, so I propose here different path: support/timepb
  • Originally I used pbtime, but it seams in protobuf they prefer to add pb as a suffix.

@robert-zaremba
Copy link
Collaborator Author

Let's decide about IsZero. If we don't want to have a special meaning for a zero time (not a nil), then I prefer to remove it.

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Generally looks good, I'm still not sure about the zero check but I see the docs make clear what "zero" means

support/timepb/cmp_test.go Outdated Show resolved Hide resolved
@robert-zaremba
Copy link
Collaborator Author

@aaronc so maybe we should remove the IsZero? (ref: #60 (comment))

@robert-zaremba
Copy link
Collaborator Author

I've added rapid tests

Copy link
Contributor

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

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

looks good, just 1 nit

support/timepb/cmp.go Show resolved Hide resolved
@aaronc
Copy link
Member

aaronc commented Feb 8, 2022

@aaronc so maybe we should remove the IsZero? (ref: #60 (comment))

I'd say let's drop IsZero. Other stuff looks good

@@ -0,0 +1,3 @@
# timepb
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a README or docs.go? Not sure what is best practice. I think the advantage of docs.go is that it shows up in pkg.go.dev like this for sub-packages:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

README shows up in pkg.go.dev as well when you go to the package page. I think we need both. Will add doc.go

support/timepb/doc.go Outdated Show resolved Hide resolved
Co-authored-by: Aaron Craelius <aaron@regen.network>
@technicallyty technicallyty merged commit 213b768 into main Feb 8, 2022
@technicallyty technicallyty deleted the robert/timepb branch February 8, 2022 17:44
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