-
Notifications
You must be signed in to change notification settings - Fork 89
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
Parsing of path-references to pages and modules #1142
Conversation
Are |
Yes! As well as many variations like |
39956f2
to
dd68498
Compare
Rebased and ready for an other review. #1150 is rebased on top. |
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.
Overall looks very good! Left a first round of comments. Not much left to review.
src/model/reference.ml
Outdated
| `End_in_slash, [] -> | ||
(* {!identifier'/identifier} *) | ||
(`TRelativePath, next_token.identifier :: components) | ||
| `End_in_slash, next_token' :: tokens' -> |
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.
Should we check here that next_token.identifier
is not ""
, and raise an error otherwise? If not, it allows references such as {!////////////////////////////////}
.
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.
Good catch! Added the error.
@@ -620,6 +629,7 @@ module rec Reference : sig | |||
[ `Resolved of Resolved_reference.field_parent | |||
| `Root of string * tag_parent | |||
| `Dot of label_parent * string | |||
| `Module_path of hierarchy |
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.
Module path can only be of the form: /libname/
. For example: {!/libname/module-M.type-t}
.
I think that could be reflected in the type?
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 guess that #1142 (comment) answers no to the question!
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 think this can happen with {!/M.type-t}
. I'm not in favor of enforcing that in the parser. The resolver has to match on the path to be able to lookup libname
, that's where the reference should be rejected.
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 finished reviewing, and have no more comments than the one already made!
Once they are resolved, it's ready to be merged.
This add 3 new kinds of references to pages and units: - Relative paths: {!./foo} {!foo/bar} - Absolute paths: {!/foo} - Paths relative to the current package: {!//foo} The last component of the path can point to a page or a root module. It can be prefixed: {!foo/page-bar} {!foo/module-Bar}. Paths can be on the right of a dot: {!foo/bar.label}. The new reference constructors are plumbed through xref2 but unimplemented.
The Page_path constructor cannot represent these references differerently: {!foo/module-Bar} {!foo/page-Bar} {!foo/Bar} The constructors Module_path and Any_path are added in the different reference types and pipeped through to Ref_tools. This also fixes the parsing of paths as label parents and add better error messages when the last component of a path isn't as expected.
'Path' was an overloaded term.
Also improve the path parsing to be more readable.
Add the parsing of references to pages and modules containing a slash:
{!./foo}
,{!/foo}
,{!//foo}
.These are not resolved yet.