-
Notifications
You must be signed in to change notification settings - Fork 90
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
Frontmatter syntax #1210
base: master
Are you sure you want to change the base?
Frontmatter syntax #1210
Conversation
src/model/semantics.ml
Outdated
@@ -561,12 +561,22 @@ let append_alerts_to_comment alerts | |||
let ast_to_comment ~internal_tags ~sections_allowed ~tags_allowed |
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 ast_to_comment
function does two orthogonal things:
- turning the parser AST into an odoc AST
- turning the parser frontmatter into an odoc frontmatter
while the two are not dependent on each other.
This results in this function being called multiple times with a made up empty frontmatter, followed by an ignore of the output frontmatter.
To me, this suggests to have two functions: ast_to_comment
(put back to its original version) and sexp_to_frontmatter
(or some other name) for the frontmatter.
That's the problem with s-expressions. Honestly I don't think you should use s-expression. They have no standard. If you really think we need something beyond simple Also I did not think a lot about but did people think about simply using OCaml attributes ? This bring back
|
@@ -209,35 +236,35 @@ let%expect_test _ = | |||
let module Trivial = struct | |||
let empty = | |||
test ""; |
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.
Would it be possible to have test print something very special in case there is a non-empty frontmatter, and otherwise use the previous output?
And another test_with_frontmatter
which output the current version of test
.
It would prevent having multiple empty parentheses which makes it hard to read in 99% of the cases.
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.
Agreed.
I take your point, though I suspect we can rely on https://github.com/janestreet/parsexp to be our definition of the 'standard' we adhere to for the question of 'what is a valid sexp'.
I'm not convinced this is better than a simple sexp. From the point of view of parity between
Going back to what this PR implements, I think I'd prefer the dune style over jbuilder style, that is, rather |
Well that's not a standard by any means, there's not even a formal grammar there :-) But more importantly it doesn't tell me how you encode dictionaries in sexps. Once you really start working with them you realize there's no such as thing as as "simple" s-expression: no one agrees on how to encode dictionaries which is exactly what your last sentence is about… We end up with "preferences", the particular one you propose being a pretty bad one from a generic s-expression processing perspective (see the linked comment). Before adding the complexity of a full blown, ill-defined and user-unfriendly serialisation format, could we perhaps start by listing exactly what is supposed to go in these front-matter for now ? Honestly I think that a couple of |
I will never remember exactly how many |
This is a straw man. Soon or later you will disable a deprecation warning and you will learn. At that point you will be so happy to discover that there is actually some form of consistency in the design of the eco-system. And even if you don't learn, it will be just one example away, it's not as if you are going to write these every day.
Newcomers are unlikely to be familiar with them, most will actually struggle with them. Newcomers would much prefer to have JSON or whatever other configuration format à la mode they are familiar with in their A reasonable assumption it so say that proficient I really don't think it's a good idea to add to the ocamldoc language an odd, non standard, s-expression syntax, used as the current configuration syntax, by one of the OCaml build tools, however dominant it may be now. Other than that, this question:
remains unanswered. |
I do not see how that is a straw man. I have not misrepresented someones argument.
I have used attributes before. I just do not remember the exact number of
They might although I do not remember struggling that much, but my argument was that they will have to learn them anyways, which is not the case for attributes which are an advanced feature that you do not need that often.
Most likely yes, but they don't have that, so they learn the dune syntax instead.
Its dominance is very relevant to the argument I am making. People are familiar with the format because dune is dominant. I care about the familiarity.
This PR is concerned with having a list of children pages. We will want to add other things later so we cannot just add a list separated by line returns. I do not know what the other stuff is, @jonludlam should be able to say more about this. |
It's a straw man to the discussion. As the user of the language you will have to learn about attributes anyways whether your remember how much In general I rather have less things to learn for mastering the language, with this proposal you are forcing me to learn a new separator ( Also there is a discussion upstream to add unit headers to source files. Before you go on inventing your own stuff, consistency with it is likely something that should be considered.
Familiar doesn't mean it's good (and is not a good measure for usability). Dominant even less…
While I can see that you would not like to have a simple line based |
Also another existing syntax that could be simply reused (an which is already parsed by odoc) is of course So this could simply be leading |
That makes sense
For the separator, this stands for everyone, but for the dune syntax you are an outlier. The fact that you write documentation of a lot of important libraries matters in this, but I am pretty sure most users learn the dune syntax long before attributes.
If they can change something as big as their config language, I do not see why we could not also change later on. I also don't think the current proposition is remotely as bad as you say it is. Sexps might not have an agreed upon way to describe dictionaries, but we can just pick one. |
Because this is a short term view. You should treat the There is absolutely no need to integrate
Neither is Honestly now that I gave a good thought to the proposal of this PR, it looks rather ridiculous. It's completely ignoring what already exists in the language. |
As far as I know, dune is also quite stable and cares a lot about retro-compatibility. Still, now that you showed the syntax for odoc tags, I agree its better (I had not read that comment before writing my previous message). |
Currently, I think:
I find the current proposal neither bad nor perfect: it syntactically forces the data to be at the top, separated from the content. Frontmatter are purely mld things. Sexp is just a serialization format, no problem with it (when it is not à la mode anymore, we'll just have a nostalgic feeling when seeing it, thinking how our QRCode based format is better). I find the tag proposal neither bad nor perfect: It reuses part of the language, but a part that was not very nicely defined (we have three indistinguishable kind of them, with bad behavior in the "block case". |
Thanks @panglesd for the list. Then yes I think that importing any full blown serialisation format into This especially since the I'm not interested in nostalgic feelings when I read an
If you look at the issue it rather looks like it's going to be a fix rather than an extension. Just define block tags as @lpw25 suggested in the linked issue, i.e. stops at the next Footnotes
|
I agree using this opportunity to fix tags is a good idea. I am not sure using a blank line to do that is the way to go though. I think the intent of block tags is to allow any odoc syntax in them, and you might want empty lines in there. |
This is likely better discussed on the actual issue. But I tend to agree with what @lpw25 mentioned there:
Or perhaps generalizing a bit: a block If people really still want block tags to span multiple blocks we could also add a convention that if you want multiple blocks you seperate them by a lone @ on its own line (rather than a blank line). |
We had a discussion about this during the recent odoc dev meeting and have come to the following
I was thinking of having 'namespaced' tags for this, in the spirit of attributes, so the tags we use would be Does this sound OK? |
I think this looks excellent. Regarding this:
I don't have a strong opinion but I feels a bit redundant to me. I'm not sure why upstream decided it was good to have |
This adds sexp-syntax for the frontmatter.
There are still things to decide : the exact syntax (there are multiple ways to structure sexps), and the errors messages.
Compatibility with previous versions of OCaml is also not done.
I am not a 100% happy about my abstractions to match on sexps, this require some thinking and I will be happy to get input on that.
I added a dependency to sexplib, which feels reasonable to me, we are using sexps so we need something to parse them (and thats hard to write because of comments and escape sequences).
I also noticed that dune has an sexps parser, which we might be able to use if they export it, and this won't add any dependency. If we get to use their sexp-matching syntax its even better.