Leonidas-from-XIV / slacko

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

Add string_of functions for various IDs. #23

Closed paurkedal closed 6 years ago

paurkedal commented 6 years ago

These are useful for serialising data without making additional requests.

Leonidas-from-XIV commented 6 years ago

What do you need these for? The API is deliberately asymetrical since there is not much use in having the identifiers be stringly typed. If you need to use them in maps or similar I'd much rather add the necessary APIs for that.

paurkedal commented 6 years ago

My use case is storing messages in an external database, and these IDs can also be used to identify the corresponding DB rows. I may see your point, though, as there is a difference between arguments supplied to the Slack API and fields returned: The former may be names while the latter, I believe will, will always be IDs. So, it's the string_of-functions are only useful to identify object references which has been though Slack, and since they are the same type, we can't make that distinction.

I think one may sometimes want to compare the part after the C, G, and D prefixes to identify the same message occurring in different context. Again, such comparison would only be reliable for object references which comes back from Slack.

So, if you are against it, I can live with the extra lookup. A cache should remove most of the overhead.

Leonidas-from-XIV commented 6 years ago

I am definitely in favour of adding compare functions to the identifier modules. This might pose a bit difficult as comparing a name with an ID might either return (incorrectly) false or require a request, thus returning an int Lwt.t which is not usually what compare functions do.

As such I would sort of keep the conversion one way for now. I am sort of thinking it would most likely be best to do the lookup directly in the of_string functions and stop bothering with resolving names afterwards. The only issue would be someone renaming e.g. a channel and expecting their old channel_of_string "#whatwasrenamed" to point to the new channel. That's possibly too much of a corner-case though.

paurkedal commented 6 years ago

One option might be something like

type +'a channel constraint 'a = [< `Id | `Name]
val channel_of_name : string -> [> `Name] channel
val channel_of_id : string -> [> `Id] channel
val resolve_channel : 'a channel -> [> `Id] channel Lwt.t
val equal_channel : [< `Id] channel -> [< `Id] channel -> bool

etc., and any inputs in the API take `a channel whereas outputs produce [`Id] channel. This would of course be an incompatible change, though maybe not too severe in practise.

Leonidas-from-XIV commented 6 years ago

I think this is an excellent idea. I'd probably release 0.14 with the jbuilder stuff first (and maybe some automation to make releases less painful), so people can use that release with minimal changes required but the \Id/`Name` distinction is a good idea.