Skip to content

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

Merged
merged 8 commits into from
Jun 9, 2022
Merged

Implement math syntax #5

merged 8 commits into from
Jun 9, 2022

Conversation

giltho
Copy link
Contributor

@giltho giltho commented Jan 26, 2022

Based on discussion in ocaml/odoc#123 and ocaml/odoc#811

Copy link
Contributor

@Julow Julow left a 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 ]
Copy link
Contributor

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.

Copy link
Collaborator

@jonludlam jonludlam Feb 24, 2022

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

@jonludlam
Copy link
Collaborator

Hi @giltho, I wanted to check whether you were planning to make the changes we suggested in the odoc PR? specifically to have {math ... } as display math. If not, I'm happy to do this myself.

Signed-off-by: Sacha Ayoun <sachaayoun@gmail.com>
@giltho
Copy link
Contributor Author

giltho commented Feb 23, 2022

@jonludlam sorry got a bit overwhelmed by work.
I was halfway through that modification, so I just finished it :)

@jonludlam
Copy link
Collaborator

Great, thanks! Wasn't meaning to hassle you :-)

@giltho
Copy link
Contributor Author

giltho commented Feb 23, 2022

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>
Copy link
Contributor

@Julow Julow left a 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 !

@dbuenzli
Copy link
Contributor

dbuenzli commented Jun 9, 2022

What happened to the enthusiastic intent @Julow ?

@jonludlam
Copy link
Collaborator

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...

@jonludlam jonludlam merged commit e6763d8 into ocaml-doc:main Jun 9, 2022
@jonludlam
Copy link
Collaborator

Thanks @giltho !

jonludlam added a commit to jonludlam/opam-repository that referenced this pull request Jul 7, 2022
CHANGES:

- New inline and display math markup (@giltho, ocaml-doc/odoc-parser#5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants