search

Leonidas-from-XIV / slacko

A neat interface for Slack
https://leonidas-from-xiv.github.io/slacko
Other
81 stars 11 forks source link

Switch timestamps to ptime. #30

Closed paurkedal closed 5 years ago

paurkedal commented 6 years ago

This changes time stamps to Ptime.t, as discussed in #28. Though, as you can see I had to duplicate quite a number of lines into the test. Maybe there is a better way.

paurkedal commented 6 years ago

My suggestion to avoid duplicating code in the test is to move the code into src/lib/interval.ml (or src/lib/private.ml). This will create a module Slacko__Internal. This will unfortunately be available also externally. A solution to ocaml/dune#427 might help in the future, but I think the name and double underscore makes it obvious enough that the module is not meant to be used outside Slacko.

This would be a solution to other duplicate definitions in tests, though I don't think I should move those in this PR. I'll start the internal.ml file with the verbatim copyright header from slacko.ml if you don't have other preferences, or wish to move the other pieces yourself first and let me rebase, or have an entirely different suggestion.

paurkedal commented 5 years ago

I hope this is still on the table, as it would be good to avoid routing issues when identifying messages.

Dune 1.2 introduced private modules, so I moved the utility function shared by the test into Slacko__Internal. I can squash the two commits if you prefer.

Leonidas-from-XIV commented 5 years ago

Thanks, I'll merge when Travis is done.

paurkedal commented 5 years ago

Since the functions from Internal is used implicitly by the ppx, we need type references to be changed accordingly to eliminate open. I did this by renaming Internal to Timestamp and changing the timestamp type to Timestamp.t. This means type definitions will be different in the mli and ml (since we don't expose Timestamp), but that's probably okay?

Leonidas-from-XIV commented 5 years ago

Yes, that's alright.

paurkedal commented 5 years ago

Great, thanks for the quick response!