mirage / functoria

A DSL to invoke otherworldly functors
ISC License
63 stars 21 forks source link

Use Ptime for time argument #160

Closed emillon closed 5 years ago

emillon commented 5 years ago

Cf https://github.com/mirage/functoria/pull/159#discussion_r222954358

cc @hannesm

Is the extra dependency OK?

Thanks!

hannesm commented 5 years ago

this looks good to me. the additional dependency (ptime) is fine with me (it only affects the functoria package, not functoria-runtime).

Drup commented 5 years ago

I'm sorry, but do we gain anything by that? We don't use Ptime here, we simply embeded the current time in a file, we don't manipulate it, and we certainly don't use a clock, posix or not.

I know we have a sort of moral obligation of using all the good modern minilibs in the ecosystem, but I would prefer if we had an actual reason to do that.

emillon commented 5 years ago

If I see time:float, I'll have several things to guess: does this expect seconds? microseconds? nanoseconds? what is the epoch? With Ptime.t this is documented by the type itself.

hannesm commented 5 years ago

an actual reason for me would be to use Ptime.pp_rfc3339 instead of the 10 lines printing a timestamp.

emillon commented 5 years ago

Good idea. I used this. The time setting code needs an extra branch because of_float_s is partial. Would it be OK to depend on ptime.clock.os and call Ptime_clock.now instead?

samoht commented 5 years ago

LGTM

Drup commented 5 years ago

an actual reason for me would be to use Ptime.pp_rfc3339 instead of the 10 lines printing a timestamp.

Ah, that's a bit more convincing to me. :)

emillon commented 5 years ago

I used Ptime_clock - it looks way better than before.

Drup commented 5 years ago

I like the current version much more than the original one, since we actually use ptime to remove a bunch of code previously needed!