-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
document Identifiable module #7
Comments
I'm weirdly at a loss for words. These are just the set of properties that we routinely want out of values used as simple identifiers, in lots of different contexts. Stringable, Pretty_printer and Sexpable are important for being able to show these identifiers to users; Comparable and Hashable are important for using them as keys in maps and hashtables. I'm oddly not sure what else to add to the comment to clarify... |
Ok so maybe it is just that simple, but no reason that couldn't be the documentation then: "This module type represents a collection of function signatures that are frequently useful together. For example, we include this signature in modules Foo, Bar, and Baz". This not useless. It saves users a few minutes trying to figure out if there is some more important purpose. (These minutes add up. I posted this issue only as an example. The whole Core/Async suite has over a hundred modules. Most people don't know which ones to understand first, and which are less important.) Another question that could be answered is: why should anyone use this module? I genuinely don't see the point of it. I happily include Stringable, Sexpable, etc, separately in places I want to support those signatures. Why should I choose Identifiable; is it just to save a bit of typing and reduce 5 include lines with 1? Good enough reason if that's it, but point is I don't immediately know that to be the case. Why not just tell us that if that's it. I still don't see what is "identifiable" about it, that it can be printed or tested for equality? "Identifiable" could mean that uniqueness is somehow assured. That is clearly not so (after spending a minute looking at the signature), so again a little explanation could save everyone some minutes. |
I think the utliity is pretty clear. If I want a new abstract identifier based on strings, I can just write:
and I'm good to go. Bundling together a very commonly used set of functionality into a single standard is quite useful. Also, we write lots of functors that demand that thier inputs are Identifiable. At a glance, I see about 1200 use-cases in our tree. Anyway, I'll add a few more lines of documentation to make the purpose of the API clearer. |
Imagine someone new to OCaml and Core, and they look at this mli. They're just lost. The example you gave is perfect; that would be excellent documentation. The fact that you get an abstract type that is otherwise the same as an existing type is really useful, and I imagine lots of people won't see that right away. |
The |
The functor does actually have a comment, with an example:
This creates a module that satisfies the Identifiable interface. It doesn't make it abstract. To do that. To do that, you need to put something into the mli. This is a fundamental thing about the language, and this doesn't feel to me like the right place to document it. |
Some more extended documentation has now been released. |
Thanks. I appreciate you spending time on an issue that you clearly feel didn't deserve any.
I stand corrected on this. This was already documented. |
I'm sorry I came off as dismissive. You had a good question, but at first I
wasn't really sure how to improve the docs. I hope the current docs are an
improvement!
…On Fri, Feb 10, 2017, 9:56 AM Ashish Agarwal ***@***.***> wrote:
Thanks. I appreciate you spending time on an issue that you clearly feel
didn't deserve any.
I also don't know what module_name is needed for.
I stand corrected on this. This was already documented.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#7 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AArqJvmK5e0ZyinTBfma9j7lXzdqlqkoks5rbHqSgaJpZM4L1O-p>
.
|
Yes, it is definitely helpful. But to be honest an important example that would really help is missing and an obvious question that everyone surely will have remains unanswered. I could expand but I feel I'll just be further harassing you on a point that is at once not really worth more of your time, and on the other hand so big that we can't resolve it over GitHub. |
Documentation for
Identifiable
includes only the uninformative comment "a signature for identifier types". I see that the signature combines a handful of useful functions and this signature is included in various other modules. However, the nameIdentifiable
suggests some more specific property that this module provides. What?The text was updated successfully, but these errors were encountered: