Skip to content

Revert "Highlight all EBNF snippets. Add missing semicolons." #14958

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 1 commit into from
Apr 25, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Apr 18, 2022

This reverts commit a60c4e7.

@pikinier20
Copy link
Contributor

Can we not revert the PR and instead replace semicolons with some other less misleading chars? I thought about dots or maybe some other unicode characters?

@odersky
Copy link
Contributor Author

odersky commented Apr 21, 2022

Can we not revert the PR and instead replace semicolons with some other less misleading chars? I thought about dots or maybe some other unicode characters?

But why? Scala does not require termination characters, so why add one for EBNF? That is super misleading. Besides, the style without termination character has been used for every Scala grammar since the start and is also the style of the official Java grammar. So, let's not mess with this!

@pikinier20
Copy link
Contributor

The need of having some termination character is related only to the fact that we use highlight.js as a highlighting tool and the rules of highlighting EBNF are then much easier. We can also revert this and have small styling issues in ebnf snippets which we can solve later in different way.

@odersky
Copy link
Contributor Author

odersky commented Apr 25, 2022

@pikinier20 Yes, let's do this. Never let your tooling dictate your documentation content.

@pikinier20
Copy link
Contributor

pikinier20 commented Apr 25, 2022

I'm gonna make a PR from this branch to stable documentation branch to have it updated there. Then we'll have a backport PR that will also change it on main branch.

First, we probably need to merge this #14994

@odersky odersky merged commit fad2600 into scala:main Apr 25, 2022
@odersky odersky deleted the revert-grammar branch April 25, 2022 14:34
ckipp01 added a commit to ckipp01/dotty that referenced this pull request Feb 6, 2023
I see that this was done in the past in
https://github.com/lampepfl/dotty/pull/14958/files, but then reverted in
scala#14958. Like many commits, there
really isn't an explanation of the revert, but from reading between the
lines I assume the `;` was the actual issue, not the syntax
highlighting. As it was pointed out, syntax.js doesn't actually support
`ebnf`. They do say they support `bnf`, but that didn't really work when I
was testing. Either way, this pr makes sure that we _do_ mark the
snippets as `ebnf`. The reason for this isn't necessarily so that we
_get_ syntax highlighting for these, but so that syntax.js doesn't infer
the wrong type of syntax and provide odd highlighting like we currently
have. This also helps to ensure screen readers know what type of
codeblock this is.

fixes scala#14697
ckipp01 added a commit that referenced this pull request Feb 7, 2023
I see that this was done in the past in
https://github.com/lampepfl/dotty/pull/14958/files, but then reverted in
#14958. Like many commits, there
really isn't an explanation of the revert, but from reading between the
lines I assume the `;` was the actual issue, not the syntax
highlighting. As it was pointed out, syntax.js doesn't actually support
`ebnf`. They do say they support `bnf`, but that didn't really work when
I
was testing. Either way, this pr makes sure that we _do_ mark the
snippets as `ebnf`. The reason for this isn't necessarily so that we
_get_ syntax highlighting for these, but so that syntax.js doesn't infer
the wrong type of syntax and provide odd highlighting like we currently
have. This also helps to ensure screen readers know what type of
codeblock this is.

fixes #14697
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.

2 participants