-
Notifications
You must be signed in to change notification settings - Fork 368
Improve some wording in the functors tutorial #2023
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
Conversation
| Check the program's behaviour using `opam exec -- dune exec funkt < dune`. | ||
|
|
||
| **Note**: The functor `IterPrint.Make` returns a module that exposes the type from the injected dependency (here first `List.t` then `Array.t`). That's why a `with type` constraint is needed. When parametrising other something not exposed by the module (and _implementation detail_), the `with type` constraint is not needed. | ||
| **Note**: The functor `IterPrint.Make` exposes the type from the injected dependency (here first `List.t` then `Array.t`) as the type `t` required of the result module. That's why a `with type` constraint is needed. When customising a type that's not exposed by the result module (i.e. an _implementation detail_), the `with type` constraint is not needed. As a further consequence of this, we could even define a local type `t` within the `struct` block for use within the implementation, and such a type `t` would not shadow the type `t` of the result module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @neuroevolutus, that note needs to be clarified. Your text is better than is initial one, do you think this improves on it:
| **Note**: The functor `IterPrint.Make` exposes the type from the injected dependency (here first `List.t` then `Array.t`) as the type `t` required of the result module. That's why a `with type` constraint is needed. When customising a type that's not exposed by the result module (i.e. an _implementation detail_), the `with type` constraint is not needed. As a further consequence of this, we could even define a local type `t` within the `struct` block for use within the implementation, and such a type `t` would not shadow the type `t` of the result module. | |
| **Note**: Modules received and returned by `IterPrint.Make` both have a type `t`. The `with type ... :=` constraint is needed to make the two `t` identical. This allows functions from the injected dependency and result module to use the same type. When the parameter's contained type is not exposed by the result module (i.e. when it is an _implementation detail_), the `with type` constraint is unnecessary. |
Regarding your last sentence:
As a further consequence, we could even define a local type
twithin thestructblock for use within the implementation, and such a typetwould not shadow the typetof the result module.
This is a bit hard to grasp because it requires 1/ figuring out what the code would look like if modified with a local t and 2/ figuring out the shadowing concern.
However, I believe you have a valid point. This is a matter that needs to be addressed. Could we address it somewhere else in the document or do we need an additional example? We may even need a complete section to address naming and shadowing concerns in relationship with functor definitions.
What is our opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the rewording. I think it's a lot more succinct and even clearer now. I made sure to commit your suggestion.
This is a bit hard to grasp because it requires 1/ figuring out what the code would look like if modified with a local t and 2/ figuring out the shadowing concern.
I agree; that is a good point 😅. I think adding a new section with a concrete example to address these concerns would be really nice. I'll work on adding this section and make a new commit to get your feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a small aside, I added a small section on empty variants to the data types tutorial to give context for the use of a singular | as a placeholder type definition.
Co-authored-by: Cuihtlauac Alvarado <[email protected]>
cuihtlauac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @neuroevolutus, this is great! I wonder if the difference between = and := type constraint is needed. But overall, you are absolutely right, this matter is missing in the current version.
| end | ||
| ``` | ||
|
|
||
| In the example above, the local type `t` does not shadow the type `t` exposed by the `with type` constraint and can be used for the implementation of the functor. However, it is generally better to avoid availing of this behaviour since it may make your code more difficult to understand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your point. I must admit I had never fallen into that pitfall and wasn't aware of it.
What do you think about turning the narrative into something like:
tfromwith typetakes over localtwhich only has a local scope?
Again, the idea is to make things more direct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree; that sounds good. It sounds clearer to me as a reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used "takes precedence over" in the rewording, but let me know if you feel it's clearer with just saying "takes over."
| Check the program's behaviour using `opam exec -- dune exec funkt < dune`. | ||
|
|
||
| **Note**: The functor `IterPrint.Make` exposes the type from the injected dependency (here first `List.t` then `Array.t`) as the type `t` required of the result module. That's why a `with type` constraint is needed. When customising a type that's not exposed by the result module (i.e. an _implementation detail_), the `with type` constraint is not needed. As a further consequence of this, we could even define a local type `t` within the `struct` block for use within the implementation, and such a type `t` would not shadow the type `t` of the result module. | ||
| **Note**: Modules received and returned by `IterPrint.Make` both have a type `t`. The `with type ... :=` constraint is needed to make the two `t` identical. This allows functions from the injected dependency and result module to use the same type. When the parameter's contained type is not exposed by the result module (i.e. when it is an _implementation detail_), the `with type` constraint is unnecessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| **Note**: Modules received and returned by `IterPrint.Make` both have a type `t`. The `with type ... :=` constraint is needed to make the two `t` identical. This allows functions from the injected dependency and result module to use the same type. When the parameter's contained type is not exposed by the result module (i.e. when it is an _implementation detail_), the `with type` constraint is unnecessary. | |
| **Note**: Modules received and returned by `IterPrint.Make` both have a type `t`. The `with type ... :=` constraint is needed to make the two `t` modules identical. This allows functions from the injected dependency and result module to use the same type. When the parameter's contained type is not exposed by the result module (i.e., when it is an _implementation detail_), the `with type` constraint is unnecessary. |
It reads funny to me to have a plural (two) with just t -- is it accurate to have t modules instead? ...or t types?
Regardless, there needs to be a comma after i.e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that. I made sure to add the comma in that part.
With regards to the t, I think it may be fine to keep it as is since the t refers to two types rather than modules per se. What are your thoughts, @cuihtlauac?
Co-authored-by: Cuihtlauac Alvarado <[email protected]>
Co-authored-by: Cuihtlauac Alvarado <[email protected]>
Co-authored-by: Cuihtlauac Alvarado <[email protected]>
Co-authored-by: Cuihtlauac Alvarado <[email protected]>
Co-authored-by: Cuihtlauac Alvarado <[email protected]>
Actually, this is something I was struggling with a bit. I realised when playing around with the code that |
| ``` | ||
|
|
||
| Such types are not ordinarily useful in OCaml programs (as they do not have any constructible values), but they can be useful as temporary placeholders when defining types in a [functor](/docs/functors#writing-your-own-functors). | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the intended use case for empty types (see https://ocaml.org/manual/emptyvariants.html for an explanation of why they were introduced). An abstract type works better as a placeholder for the use case you are describing. I would thus avoid mentioning empty variant types before the GADT introduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. That would make sense. I'll revert the corresponding commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've edited the Binary functor example to make elt and t abstract types.
Thanks for the link to the manual! I'll check that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To give a better example of what I meant by "abstract type are better placeholder", if you write
type empty = |
let f ([]: empty list) = ()the function f is total: the only list of empty values is the empty list. But this is property is only true for empty types. In other words, by using empty types as placeholders one might accidentally capture properties that are only true for empty types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I think I better understand now. As an aside, are there any examples in the wild where using empty types and refutation cases for them come in handy when it comes to GADTs? I think I'm still fuzzy on the motivation for empty types.
Co-authored-by: Cuihtlauac Alvarado <[email protected]>
I believe we can move forward with this PR without exposing the difference between the |
cuihtlauac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Thnaks @neuroevolutus
Thank you so much for all the feedback! I'm so happy to have had the chance to contribute. |
This PR adds some minor grammatical and wording changes to the functors tutorial that will hopefully make things clearer for new users of OCaml.
In particular, I made some changes to the aside about the
with typeconstraint that may need particular review to ensure I did not state something that is semantically incorrect.