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 zulu time formatting helpers #321

Open
ransombriggs opened this issue Dec 5, 2017 · 3 comments
Open

Add zulu time formatting helpers #321

ransombriggs opened this issue Dec 5, 2017 · 3 comments

Comments

@ransombriggs
Copy link

To get Time objects into a zulu formatted string various pliny apps have defined identical helpers that could be consolidated in pliny. I started this issue rather than filing a PR since there are some approach questions that we should probably discuss.

The time_v3 helpers are usually defined in a module which is mixed into serializers and sometimes mediators if they are publishing events. There are currently two different implementations I have seen.

def time_v3(time)
  time&.getutc&.strftime("%Y-%m-%dT%H:%M:%SZ")
end
def time_v3(time, precision: 0)
  time&.getutc&.iso8601(precision)
end

The second version has the extra flexibility of having sub second precisions which I believe the app uses for event timestamps. The only problem with this method is that it does not work for DateTime objects where as the first implementation does.

[1] development(main)> Time.now.getutc.iso8601
=> "2017-11-29T16:45:40Z"
[2] development(main)> DateTime.now.getutc.iso8601
=> "2017-11-29T16:45:45+00:00"

I guess I would lobby for the strftime implementation at this point so that it would not be confusing when used with DateTime objects.

Also, time_v3 is probably not the best name for this method in pliny since it is tied to the idea of the heroku api v3 implementation instead of zulu formatted dates.

Finally, would this be better as an optional mixin on Time and DateTime instead?

/cc @gudmundur

@gudmundur
Copy link
Member

@ransombriggs Thanks for the thorough details on this issue.

As for which implementation to use, it's a tough one. I'm not sure what the reasoning behind the time precision is, maybe @rwz can get into that? As Pliny defaults to using DateTime in serializers, I'd say we should stick to that. Higher precision can be provided for the event envelope specifically.

I agree that the time_v3 name isn't the best, how about time or zulu_time?

I think we should leave it as a class method on the serializer base class, rather than mixing into Time or DateTime.

@ransombriggs
Copy link
Author

@gudmundur I am down with having it available as a class method on the serializer base class, but would like to continue leaving it as a module that I mix into the serializer base. I have seen usage in mediators as well, specifically for submitting jobs with a string timestamp and would like to explicitly include the helper there.

@ransombriggs
Copy link
Author

@gudmundur pushed up #322

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

No branches or pull requests

2 participants