-
Notifications
You must be signed in to change notification settings - Fork 15
Implement math syntax #5
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
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.
Why is the Raw_markup
syntax not enough for your use case ? Is it just to provide a nicer syntax ?
Anyway, I think the parsing of {%math: %}
shouldn't be modified to allow the PR on ocaml/odoc to live whether or not this PR is accepted.
Even better, {m ... }
can simply be a sugar for the raw markup syntax, to avoid introducing breaking changes.
src/ast.ml
Outdated
@@ -22,7 +22,8 @@ type inline_element = | |||
| `Styled of style * inline_element with_location list | |||
| `Reference of | |||
reference_kind * string with_location * inline_element with_location list | |||
| `Link of string * inline_element with_location list ] | |||
| `Link of string * inline_element with_location list | |||
| `Math of bool * 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.
I'm not a fan of the bool to distinguish between the two forms. I think it's not necessary to recognize {%math:
specifically, `Math
should only correspond to the {m
syntax.
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'm also not a fan of bools, though I do think this should be used for both display and inline math (I think @Julow's comment refers to the older idea of reusing {%math:...}
that we've since ruled out). Could we have a type math_style = [ `Inline | `Display ]
instead?
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 the distinction between {m ...}
and {math ...}
is that one is inline and the other is a block. These should be two different constructors in the AST: one in inline_element
and a different one in nestable_block_element
. I propose to name them `Math_span
and `Math_block
to reuse the naming of code spans and code blocks.
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.
Yep, I'm going to go with the separated `Math_span
and `` Math_block
I think, it does make sense to construct it symmetrically with code spans and blocks
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.
Makes sense to me!
Hi @giltho, I wanted to check whether you were planning to make the changes we suggested in the odoc PR? specifically to have |
Signed-off-by: Sacha Ayoun <sachaayoun@gmail.com>
@jonludlam sorry got a bit overwhelmed by work. |
Great, thanks! Wasn't meaning to hassle you :-) |
Oh not at all, didn't mean that! It was definitely the right thing to do :) |
Signed-off-by: Sacha Ayoun <sachaayoun@gmail.com>
Signed-off-by: Sacha Ayoun <sachaayoun@gmail.com>
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.
Sorry for the delay, let's merge !
What happened to the enthusiastic intent @Julow ? |
oh boy, I looked at the date of @Julow's comment and thought '10th March? that's only a couple of weeks ago, right?' -- I'm sure time is passing faster than it used to do... |
Thanks @giltho ! |
CHANGES: - New inline and display math markup (@giltho, ocaml-doc/odoc-parser#5)
Based on discussion in ocaml/odoc#123 and ocaml/odoc#811