interagent / pliny

An opinionated toolkit for writing excellent APIs in Ruby.
MIT License
800 stars 77 forks source link

Add zulu time formatting helpers #321

Open ransombriggs opened 6 years ago

ransombriggs commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@gudmundur pushed up https://github.com/interagent/pliny/pull/322