Skip to content
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

Introduce new syntax for explicit block type #385

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

NathanReb
Copy link
Contributor

This introduce a new, more explicit labels block-type=... which is meant to replace the four separate labels cram, ocaml, toplevel and include in the future.

It is added for a transition period at the end of which the four labels of the apocalypse should be removed in favor of this new one.

I'm opening this as a draft as one of the test relies on MDX reporting multiple errors at once which I thought we had but turns out we still haven't so I'll work on this next to unblock this PR.

Other than that it is ready for review!

Copy link
Collaborator

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

block-type is too long for something that must be written very often. block- is redundant but I think it can be shorter, for example a single character: $ocaml, $toplevel. $ is used in the extended label syntax.

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Looks good so far. Question is how we want to handle the migration:

  • Should ocaml blocks be converted to block-type=ocaml blocks
  • Should we do this now or should we support both in parallel?

@@ -0,0 +1 @@
[mdx] Fatal error: File "test-case.md", line 6: invalid code block: Label `block-type` requires a value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The error for block-type=invalid should probably have its own test case, since we exit on the first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes hence me working on multiple error reporting before undrafting this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I should rewrite the tests to use the new labels syntax!

@NathanReb
Copy link
Contributor Author

The label will never be mandatory and I think we should probably keep the inference. It just provides a way to be explicit about things, providing better error messages. The label must not be confused with the language header. We can switch the label to type if the length really is a problem.

CHANGES.md Outdated Show resolved Hide resolved
@NathanReb
Copy link
Contributor Author

for example a single character: $ocaml, $toplevel. $ is used in the extended label syntax.

The idea for this new "syntax" here is to make the label more easily readable and understandable to anyone. I agree block- was redundant and removed it but I believe having a regular label syntax that state what this value is about helps understanding what it does, looking up the label in documentation (which I'm currently writing and is part of why I worked on this feature), etc...

I think the 4 extra characters don't do a lot of harm but they do simplify things for the newcomers to projects using MDX quite a lot!

@NathanReb
Copy link
Contributor Author

I'll undraft once #389 is merged!

This introduce a new, more explicit labels `block-type=...` which
is meant to replace the four separate labels `cram`, `ocaml`, `toplevel`
and `include` in the future.

It is added for a transition period at the end of which the four labels
of the apocalypse should be removed in favor of this new one.

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb
Copy link
Contributor Author

Just rebased!

@NathanReb NathanReb marked this pull request as ready for review August 3, 2022 15:28
NathanReb and others added 2 commits August 4, 2022 11:45
Signed-off-by: Nathan Rebours <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

The code is good, my suggestions are mostly just from the renaming of block-type to type. Just overlooked details.

What I would however like to see would be some free-text documentation (e.g. in the README) about this, maybe with some advice whether and when to use ocaml, type=ocaml or inference, to make this a bit more discoverable for users as well as clearer which of the various choices make the most sense in their particular situation.

CHANGES.md Outdated Show resolved Hide resolved
test/bin/mdx-test/expect/block-type/test-case.md Outdated Show resolved Hide resolved
test/bin/mdx-test/expect/block-type/test-case.md Outdated Show resolved Hide resolved
test/bin/mdx-test/expect/block-type/test-case.md.expected Outdated Show resolved Hide resolved
test/bin/mdx-test/failure/block-type-value/test-case.md Outdated Show resolved Hide resolved
test/bin/mdx-test/failure/block-type-value/test-case.md Outdated Show resolved Hide resolved
@NathanReb
Copy link
Contributor Author

Indeed, it seems to be good to clarify why we added explicit block types in the first place:

The main motivation was that people were sometime confused at error messages or behaviour. For instance they want to write a toplevel block but forget the leading # and can't figure out why it doesn't behave the way they want it to because MDX just treated it as an OCaml block. By adding the explicit type=toplevel, they would get an error about the misisng the #.
There are a few other such examples but I can list them from the top of my head.

The other reason is clarity of the semantic. Using explicit labels, which are easy to look up (especially once we finish the manual), makes it clear what the intent of the block is and easier for newcomers to understand how MDX will behave.

The straight ocaml, toplevel, etc. form should be gradually removed as it doesn't help much with clarity. The explicit label name can be looked up in the documentation and will inevitably lead to describe what the four types of block are.

My personal recommendation would be to always use the explicit type label as when writing an MDX block, one usually knows what they want to do with it. I understand some people will prefer conciseness to clarity hence why we'll keep block inference.

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb
Copy link
Contributor Author

I just rebased and hopefully fixed the issues you pointed out. The CI failure on alpine seems unrelated!

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