Module type alias #703
Merged
Module type alias #703
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Don't expand module types if they're just a `Path` to another one - If a module type is a `Path` to another, add in a `AliasModuleType` constructor, which indicates that URLs should point to the aliased module type rather than the original
jonludlam
force-pushed
the
module-type-alias
branch
from
7838de3 to
07faa71
Compare
jonludlam
force-pushed
the
module-type-alias
branch
from
07faa71 to
35c51bf
Compare
jonludlam
force-pushed
the
module-type-alias
branch
2 times, most recently
from
1d63c83 to
564d13a
Compare
jonludlam
force-pushed
the
module-type-alias
branch
from
564d13a to
c51d0a7
Compare
Julow
reviewed
Jul 14, 2021
| @@ -314,7 +314,7 @@ and module_type : Env.t -> ModuleType.t -> ModuleType.t = | |||
| let expr = | |||
| match m.expr with | |||
| | None -> None | |||
| | Some e -> Some (module_type_expr env sg_id e) | |||
| | Some e -> Some (module_type_expr env sg_id ~expand_paths:false e) | |||
There was a problem hiding this comment.
Why do we need to disable expansion here?
There was a problem hiding this comment.
The only time we actually want to consider skipping the expansion of paths is when handling a ModuleType signature item - we never want to for for Module items, and we never want to when recursing.
| @@ -334,6 +332,8 @@ and is_resolved_module_type_hidden : Resolved.module_type -> bool = function | |||
| | `ModuleType (p, _) -> is_resolved_parent_hidden ~weak_canonical_test:false p | |||
| | `SubstT (p1, p2) -> | |||
| is_resolved_module_type_hidden p1 || is_resolved_module_type_hidden p2 | |||
| | `AliasModuleType (p1, p2) -> | |||
| is_resolved_module_type_hidden p1 || is_resolved_module_type_hidden p2 | |||
There was a problem hiding this comment.
Should it be just is_resolved_module_type_hidden p1 ? There is a few conditions similar to "if p2 is hidden then use p1 else use p2", so does it matter if p2 is hidden here ?
| @@ -24,6 +23,7 @@ module rec Resolved : sig | |||
| | `Identifier of Identifier.ModuleType.t | |||
| | `ModuleType of parent * ModuleTypeName.t | |||
| | `SubstT of module_type * module_type | |||
| | `AliasModuleType of module_type * module_type | |||
There was a problem hiding this comment.
Can you add a comment saying which is which ?
| (i :> Id.Signature.t) = id | ||
| in | ||
| let expansion_needed = self_canonical || hidden_alias in | ||
| if expansion_needed then |
There was a problem hiding this comment.
The expansion_needed code only uses generic types, perhaps it can be shared with the module case ?
| sg.items | ||
|
|
||
| (* Really cut-down reference lookup! *) | ||
| let rec handle_ref : |
There was a problem hiding this comment.
A simpler implementation might be to open the signature into an empty environment and use the env instead of implementing lookups.
jonludlam
added a commit
to jonludlam/opam-repository
that referenced
this pull request
Oct 5, 2021
CHANGES: Breaking changes - Remove odoc-parser into a separate repository (@jonludlam, ocaml/odoc#700) Additions - OCaml 4.13 support (@Octachron, ocaml/odoc#687, ocaml/odoc#689) - Better errors/warnings (@Julow, ocaml/odoc#692, ocaml/odoc#717, ocaml/odoc#720, ocaml/odoc#732) - ModuleType 'Alias' support (@jonludlam, ocaml/odoc#703) - Improved test suite (@lubega-simon, ocaml/odoc#697) - Improved documentation (@lubega-simon, @jonludlam, ocaml/odoc#702, ocaml/odoc#733) - Strengthen module types (@jonludlam, ocaml/odoc#731) Bugs fixed - `uwt` now can be documented (@jonludlam, ocaml/odoc#708) - Fix resolution involving deeply nested substitutions (@jonludlam, ocaml/odoc#727) - Fix off-by-one error in error reporting (@asavahista, ocaml/odoc#736)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement module type aliases.
When there is a module type that is simply an alias to another module type then we don't expand it.