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

docs: broken highlighting in syntax summary for " #14697

Closed
unkarjedy opened this issue Mar 16, 2022 · 10 comments · Fixed by #14747 or #16837
Closed

docs: broken highlighting in syntax summary for " #14697

unkarjedy opened this issue Mar 16, 2022 · 10 comments · Fixed by #14747 or #16837

Comments

@unkarjedy
Copy link
Contributor

https://dotty.epfl.ch/docs/reference/syntax.html

image

@unkarjedy unkarjedy added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 16, 2022
@pikinier20
Copy link
Contributor

The highlight.js interpreted this snippet as bash. According to https://highlightjs.org/static/demo/ it supports BNF but unfortunately no EBNF. We can try BNF highlighting and if it'll be still broken, disable highlighting.

@pikinier20
Copy link
Contributor

Another thing is that this page is moved to: https://dotty.epfl.ch/docs/internals/syntax.html where the problem is solved. Shouldn't we remove the page from reference and add redirect to updated version?

@julienrf
Copy link
Contributor

Good point @pikinier20. Yes, we need to figure out why there are two occurrences of this file, and which one should be kept.

@odersky
Copy link
Contributor

odersky commented Mar 19, 2022

There are two syntax.md pages, one in reference and one in internals. The internals page contains also experimental features supported by the compiler and sketches a translation into trees.

@odersky odersky added area:documentation and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 19, 2022
@julienrf
Copy link
Contributor

julienrf commented Mar 21, 2022

I believe that keeping only one file to maintain would be simpler. Currently, if someone contributes a fix to reference/syntax.md we have to remember to tell them to also fix it under internals/syntax.md.

Would it be possible to keep only one file? That file would include the experimental features.

@odersky
Copy link
Contributor

odersky commented Mar 21, 2022

I think we need two files. One is docs for the official language reference, the other is docs for our internal implementation. Syntax changes should be rare and well thought through. Changing two syntax.md files is the least of our worries. We also have to change scalameta, ammonite, IntelliJ and all the other tools. That's the real burden.

@julienrf
Copy link
Contributor

OK, then to fix the original issue we should just follow @pikinier20’s proposal: try marking the snippet as BNF code, and if it does not work mark it as plain text.

@unkarjedy
Copy link
Contributor Author

I think we need two files. One is docs for the official language reference, the other is docs for our internal implementation.

From what I saw it's mostly about some small note/comment to some BNF rule.
Otherwise the files looks the same

Some draft idea:
As I understand there is some control of how *.md files are preprocessed and rendered
(somewhere around here ?)
Those implementation notes could be inserted in a single reference/syntax.md.
syntax.md could be rendered in two modes then: with comments left and without the comments.

The comments could be marked using some special reserved marker, e.g. <<<< or ////, which would be removed on rendering.

### Types
```ebnf
Type              ::=  FunType
                    |  HkTypeParamClause ‘=>>’ Type                             ////LambdaTypeTree(ps, t)
                    |  FunParamClause ‘=>>’ Type                                ////TermLambdaTypeTree(ps, t)
                    |  MatchType
                    |  InfixType ;
FunType           ::=  FunTypeArgs (‘=>’ | ‘?=>’) Type                          ////Function(ts, t)
                    |  HKTypeParamClause '=>' Type                              ////PolyFunction(ps, t) ;

@julienrf
Copy link
Contributor

The Scaladoc processing logic can not really be special-cased for the Scala 3 documentation only, as it aims to be a general documentation generator.

However, it might be possible to share common content by leveraging the template engine, I guess?

@odersky odersky reopened this Apr 18, 2022
@odersky
Copy link
Contributor

odersky commented Apr 18, 2022

Reopened since a commit was reverted by #14958

ckipp01 added a commit to ckipp01/dotty that referenced this issue 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 issue 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
@Kordyjan Kordyjan added this to the 3.1.3 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment