-
Notifications
You must be signed in to change notification settings - Fork 101
Odoc parser opam package #685
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
While the opam package has been split into odoc_parser and odoc, the esy build remains one. Package build and install steps have been amended for this.
Need to figure out how to fix |
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 been able to fix opam-dune-lint
at the cost of increase Dune's version to 2.8: jonludlam#36
maintainer: "Jon Ludlam <jon@recoil.org>" | ||
dev-repo: "git+https://github.com/ocaml/odoc.git" | ||
|
||
synopsis: "Parser for OCamldoc" |
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 be "Odoc" right ? Same in the description below.
It's actually parsing an unspecified language that looks like ocamldoc.
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 of ocamldoc
as referring to the language and the tool. The sense I'm trying to convey here is that it's the parser for the language - though you're right, there are extensions beyond what the original ocamldoc language specified.
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 would argue we should transition to "the odoc markup language" (or a more catchy name). We already broke compat in various ways, and the distance is only going to grow with time.
pos_cnum = span.end_.column; | ||
} | ||
in | ||
Location.{ loc_start; loc_end; loc_ghost = false } |
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.
Shouldn't we continue to use a custom location type ?
compiler-libs
is a big dependency and is used for just that- we already have custom types for that (
span
andwith_location
) pos_bol
is set to0
because we don't have the value- it's not used
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.
The intention here is a convenience for users of the library, though it needs a bit more work. When we first call Odoc_parser.parse_comment
we pass in a Lexing.position
to indicate the start position of the comment. The intent here is to be able to turn our internal Loc.t
types back into the original type, more-or-less. The use of Location.t
as a return type is because it conveniently represents a span between two Lexing.position
values. While it does bring in a dependency on compiler-libs
, I think it's pretty unlikely anyone using this library isn't already using compiler-libs
, though we could always return a pair of Lexing.positions
instead. However, in order to do this correctly we need to stash the original Lexing.position
away and keep it for this operation. I'll give this a try and see how it looks.
As an example of where this function would be useful, see here: https://github.com/janestreet/ppx_js_style/blob/master/src/ppx_js_style.ml#L393-L432
type nestable_block_element = | ||
[ `Paragraph of inline_element with_location list | ||
| `Code_block of string | ||
| `Code_block of string option * string |
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.
Some comments from #678 (comment)
- Should we parse the "language" field ? (the first word)
We might want to use it for syntax highlighting. This feature is targetted at Mdx, which uses this field. - Both arguments should have a location attached.
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 question. I've currently done the minimum possible here in the parser, in the spirit of how references are parsed, though this and the stripping of whitespace both seem reasonable to me.
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.
and I agree with the location comment, fill fix, thanks!
let raw_markup_target = | ||
([^ ':' '%'] | '%'+ [^ ':' '%' '}'])* '%'* | ||
let code_block_meta = | ||
([^ '['])* |
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 parser doesn't allow arbitrary strings. I propose this syntax:
Optionally wrap with curly braces: {{@ocaml foo=[]}[ ... ]}
.
Closing brace can be escaped \}
, no balancing because it would clash with the other escape.
Not using quotes because it wouldn't be clear where they are allowed, this is consistent with other syntaxes (eg. links), I also thing quotes would be more common in the payload.
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 PR just does what we agreed in the dev meeting. Since the first user of this field will be mdx, do you happen to know whether this is going to be an immediate problem? That is, whether there are any square brackets in any of the current uses of this field in md files?
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 mdx only need to be able to read file names, "relation", package names, commas, version numbers and pre-defined strings. Escaping isn't needed by Mdx for now.
IMO, we should always think about escaping when writing a language that accepts arbitrary strings unless we are fine with writing "accept any string that doesn't contain '['" in the doc.
{| | ||
((output | ||
(((f.ml (1 0) (1 46)) | ||
(code_block "ocaml env=f1 version>=4.06 " "code goes here")))) |
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.
Trailing whitespaces should be removed.
- The parser package can't its tests run because of a circular dependency with ppx_expect. - Run the tests from Odoc's build script. This is the build script that Dune would have generated. However: - Increase the required Dune version to 2.8. Which breaks esy.
I think we've spent enough time struggling with the "parser library and odoc in the same repo" problem. I propose to put the parser into We can always remerge them later if we like! |
No description provided.