Conversation
|
this looks good to me. the additional dependency (ptime) is fine with me (it only affects the functoria package, not functoria-runtime). |
|
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. |
|
If I see |
|
an actual reason for me would be to use Ptime.pp_rfc3339 instead of the 10 lines printing a timestamp. |
|
Good idea. I used this. The time setting code needs an extra branch because |
|
LGTM |
Ah, that's a bit more convincing to me. :) |
lib/functoria_misc.ml
Outdated
| begin | ||
| match Ptime.of_float_s (Unix.gettimeofday ()) with | ||
| | Some t -> t | ||
| | None -> failwith "cannot convert current time" |
There was a problem hiding this comment.
Can we call directly Ptime_clock.now () here, please.
|
I used |
|
I like the current version much more than the original one, since we actually use |
| ((name functoria) | ||
| (public_name functoria) | ||
| (libraries (unix cmdliner rresult fmt astring fpath)) | ||
| (libraries (unix cmdliner rresult fmt astring fpath ptime.clock.os)) |
There was a problem hiding this comment.
not sure about best current practises: should ptime be included here as well, or is transitivity (via ptime.clock.os) sufficient? I'm in favour of the former.
There was a problem hiding this comment.
in general I agree that transitive dependencies are a potential source of bugs, but within a single ocamlfind lib it's probably acceptable.
CHANGES: * compute all transitive opam dependencies for info (mirage/functoria#151, by @hannesm) * support pin-depends in generated opam file (mirage/functoria#163, by @hannesm) * use dune as build system (mirage/functoria#158, by @emillon) * use Ptime for time printing (mirage/functoria#160, by @emillon) * inject global arguments into generated header (mirage/functoria#159, by @emillon) * add Functoria_key.add_to_context (mirage/functoria#161, by @emillon) * output opam2 files (mirage/functoria#157, by @hannesm)
Cf #159 (comment)
cc @hannesm
Is the extra dependency OK?
Thanks!