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

chore(syntax-highlight): add nolint aliases for html/js #7017

Merged
merged 3 commits into from
Sep 6, 2022

Conversation

teoli2003
Copy link
Member

This PR replaces the Prism alias for jssyntax with jsnolint, and adds htmlnolint.

jssyntax was added in #6682, but we decided not to use it as it is too semantic, and we wanted to avoid a potentially long list of semantic aliases.

The idea is to have such code blocks styled by Prism with the right color scheme but skipped when linter run, like Markdownlint, or Prettier, as these blocks are not valid (for example our syntax box), or should not be formatted by linters (like examples of invalid code, or TML templates that require an unusual alignment).

In a meeting a couple of weeks ago, I understood that we have a consensus on these two names (we need htmlnolint for some templates in the Learning Area).

So, first: do we have a consensus? @Rumyra @wbamberg @Josh-Cena @Elchi3 can you approve to mark your approval?

Once this is confirmed, let's ping the devs for a review.

Thank you all! (Consensus-decision making is sometimes a hard task, but it leads to much better solutions!)

Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

I give my approval on any kind of nolint marker. I don't care about which—we can always migrate later with a trivial search&replace. This seems to be the easiest for now, without forking all the parsers.

Co-authored-by: Onkar Ruikar <87750369+OnkarRuikar@users.noreply.github.com>
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, but as jsnolint looks like jsonlint with a typo, any chance of adding a hyphen (js-nolint, html-nolint)? Otherwise folks will regularly have to look twice when encountering occurrences of jsnolint.

@caugner caugner changed the title Add jsnolint and htmlnolint to Prism aliases chore(syntax-highlight): add nolint aliases for html/js Aug 30, 2022
@OnkarRuikar
Copy link
Contributor

OnkarRuikar commented Aug 30, 2022

If we are willing to reconsider the strategy I would like to suggest using ```js nolint for following reasons:
Pros:

  • It is more sensible from documentors' perspective as we are doing this to just avoid linting. They'll have to add just nolint to exiting blocks.
  • As @Josh-Cena pointed removing js from code blocks stops the syntax highlighting in local IDEs. And it is important because "Syntax highlighting tremendously reduces the cognitive load to mentally parse the source code."
  • It saves us from creating following in the future: cssnolint, jsonnolint, wasmnolint, phpnolint etc.
    We'll simply have to add nolint when we support new linter e.g. ```css nolint, ```json nolint
  • No change will be required in yari code. This is not a big issue though, but it'll avoid the dependency on yari for each language just to add the auxiliary linting feature.
  • In future we can quickly start linting new languages, without waiting for consensus on the tag name etc.

Cons:

  • "forking all the parsers"

If "forking all the parsers" hear means modifying linters' codes to make them recognize nolint (or jsnolint) on code fences to skip linting then we can use different approach:

Every linter supports at least two way of linting things:
A. CLI: provide code file path and config file and it lints the entire file
B. API: provide code snippet as text along with config object

We can write a wrapper around, for example ESLint, linter where it'll read provided .md file and pickup code blocks with the language and without nolint and feed it to the linter one by one using the linter's API (the option B) and log the errors.

This will also solve the problem of linting the live samples without adjusting exiting content. We use same logic which yari uses to combine live sample snippets and feed the whole code to the linter.
Only complex part will be to map error line numbers to the original .md file.

The same wrapper can support any language linters in future. All we have to do is import it's library and use the API.
This way we don't have to tweak every linter to support our tags(jsnolint, js nolint, htmlnolint).

This way we can also lint code blocks in different sections with different sets of rules. For example syntax sections with less strict rules, live samples and example section code with almost all the rules.


Forgive me if I've repeated things by being out of context.

@teoli2003
Copy link
Member Author

I'm ok with js-nolint, html-nolint.

If we can make js nolint work, I think it would be a great thing. But as it looks like some significant work, I'm not sure we want to delay this more.

We still need to gather more experience to reach a point where Prettier can be run automatically.

Also, it was pointed out earlier by Josh-Cena, if we go swap js-nolint to js nolint it will be pretty easy to fix.

Let's wait for more feedback here. (Meanwhile, I'll change the name)

@OnkarRuikar
Copy link
Contributor

If we can make js nolint work ... But as it looks like some significant work

I have no idea if tweaking the Prettier parser is simpler option. But making Prettier understand our live samples is certainly not easy. Unless we are realigning our content.

@caugner
Copy link
Contributor

caugner commented Aug 31, 2022

  • As @Josh-Cena pointed removing js from code blocks stops the syntax highlighting in local IDEs.

IMO this ☝️ is the main blocker, so I wonder what is more important:

  • Having syntax highlighting for these code blocks, or
  • Having an easier way of marking a code block as nolint (it is already possible with <!-- prettier-ignore-start/end --> comments, isn't it?).

@Josh-Cena
Copy link
Member

Having syntax highlight is not a big deal in the short term, unless we plan to use nolint immediately everywhere (including live examples). At least syntax examples are short and I can live without highlighting.

And yes, prettier-ignore and eslint-skip already work.

@teoli2003
Copy link
Member Author

teoli2003 commented Sep 1, 2022

I think that the ideal solution would be:

As info strings are standard Markdown (CommonMark at least), we should submit upstream an improvement to Prettier (or other linters), allowing the tool to ignore fences followed by specific info strings (configurable).

Then we could do the following:

```js nolint

or even

```js example-bad

That way:

  • We avoid magic comments (we want to tell our new editors: We use Markdown, go!). From our editors' point of view, we do pure Markdown.
  • We have syntax highlighting on both the site and in IDEs, without configuration.
  • We have control over the info string name and flexibility in using them.
  • We do not maintain a fork of Prettier or other linter.
  • We have given back to the community and improved a useful tool.

Should we try this? If we agree, we can contact the maintainer ahead of the work to get his opinion. I've not looked if the parser already detects info strings but if so, it shouldn't be too complex.

@caugner caugner self-assigned this Sep 1, 2022
@OnkarRuikar
Copy link
Contributor

Prettier does receive the extra info string from its parser:

```js nolint
foo()
```

// For above code block it gets following object:
{
  type: 'code',
  lang: 'javascript',
  meta: 'nolint',
  value: 'foo()'
}

The convention is to use first word as a language and rest as extra info (meta).
We need to request them to have a configurable word in meta part which can be used to skip the code block.


With above knowledge, temporary workaround could be to write a wrapper:

  • That will replace ```js nolint with ```js-nolint before feeding a file to the Prettier.
  • Without a valid language name the block will not be linted.
  • The wrapper will again replace ```js-nolint with ```js nolint before saving the file.

@caugner caugner marked this pull request as draft September 1, 2022 12:15
build/syntax-highlight.ts Outdated Show resolved Hide resolved
@caugner
Copy link
Contributor

caugner commented Sep 2, 2022

How about we merge this PR with hyphens for readability, and migrate to js nolint and html nolint as soon as the prettier plugin has been implemented and configured?

@teoli2003 @Elchi3 @Josh-Cena @OnkarRuikar Please leave a 👍 if you agree and 👎 if you have objections.

@Josh-Cena
Copy link
Member

Strong agree—let's not block this for some far future visions.

@caugner caugner marked this pull request as ready for review September 6, 2022 21:06
teoli2003 added a commit to mdn/content that referenced this pull request Sep 13, 2022
* Tag syntax code blocks with js-nolint, part 9

Adding to #19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/ndefrecord/torecords/index.md

* Update files/en-us/web/api/nodelist/entries/index.md

* Update files/en-us/web/api/nodelist/foreach/index.md

* Update files/en-us/web/api/nodelist/keys/index.md

* Update files/en-us/web/api/nodelist/values/index.md

* Update files/en-us/web/api/presentationrequest/start/index.md

* Update files/en-us/web/api/relativeorientationsensor/relativeorientationsensor/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
teoli2003 added a commit to mdn/content that referenced this pull request Sep 13, 2022
* Tag syntax code blocks with js-nolint, part 10

Adding to #19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/rtcicetransport/getremoteparameters/index.md

* Update files/en-us/web/api/rtcpeerconnection/addstream/index.md

* Update files/en-us/web/api/rtcpeerconnection/close/index.md

* Update files/en-us/web/api/rtcpeerconnection/getidentityassertion/index.md

* Update files/en-us/web/api/rtcpeerconnection/getreceivers/index.md

* Update files/en-us/web/api/rtcpeerconnection/getsenders/index.md

* Update files/en-us/web/api/rtcpeerconnection/removestream/index.md

* Update files/en-us/web/api/rtcpeerconnection/removetrack/index.md

* Update files/en-us/web/api/rtcrtpsender/replacetrack/index.md

* Update files/en-us/web/api/selection/setbaseandextent/index.md

* Update files/en-us/web/api/serviceworkercontainer/startmessages/index.md

* Update files/en-us/web/api/storagemanager/persist/index.md

* Update files/en-us/web/api/storagemanager/persisted/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
teoli2003 added a commit to mdn/content that referenced this pull request Sep 13, 2022
* Tag syntax code blocks with js-nolint, part 11

Adding to #19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/svgpoint/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
teoli2003 added a commit to mdn/content that referenced this pull request Sep 13, 2022
* Tag syntax code blocks with js-nolint, part 12

Adding to #19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/window/showdirectorypicker/index.md

* Update files/en-us/web/api/webgl_multi_draw/multidrawarraysinstancedwebgl/index.md

* Update files/en-us/web/api/webgl_multi_draw/multidrawarrayswebgl/index.md

* Update files/en-us/web/api/webgl_multi_draw/multidrawelementsinstancedwebgl/index.md

* Update files/en-us/web/api/webgl_multi_draw/multidrawelementswebgl/index.md

* Update files/en-us/web/api/webglrenderingcontext/attachshader/index.md

* Update files/en-us/web/api/webglrenderingcontext/detachshader/index.md

* Update files/en-us/web/api/window/showdirectorypicker/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
teoli2003 added a commit to mdn/content that referenced this pull request Sep 13, 2022
* Tag syntax code blocks with js-nolint, part 13

Adding to #19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/xrwebgllayer/antialias/index.md

* Update files/en-us/web/api/xrwebgllayer/getnativeframebufferscalefactor/index.md

* Update files/en-us/web/api/xrwebgllayer/getnativeframebufferscalefactor/index.md

* Update files/en-us/web/http/proxy_servers_and_tunneling/proxy_auto-configuration_pac_file/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
teoli2003 pushed a commit to mdn/content that referenced this pull request Sep 13, 2022
Adding to #19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017
teoli2003 pushed a commit to mdn/content that referenced this pull request Sep 13, 2022
Adding to #19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017
teoli2003 pushed a commit to mdn/content that referenced this pull request Sep 13, 2022
Adding to #19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017
@Josh-Cena
Copy link
Member

Josh-Cena commented Sep 13, 2022

For anyone interested in editor highlighting, I found this gem:

https://github.com/mjbvz/vscode-fenced-code-block-grammar-injection-example

We can build a VS Code extension to support more language tags even if we don't migrate to using metastring.

OnkarRuikar added a commit to OnkarRuikar/content that referenced this pull request Sep 13, 2022
Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017
OnkarRuikar added a commit to OnkarRuikar/content that referenced this pull request Sep 15, 2022
- Updating example-bad code blocks with the new tag. Refer: mdn/yari#7017
teoli2003 added a commit to mdn/content that referenced this pull request Sep 15, 2022
* Tag `example-bad` blocks with html-nolint

- Updating example-bad code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/learn/accessibility/html/index.md

* Update files/en-us/learn/accessibility/html/index.md

* Removed suggested no nolints

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>

* remove a trailing space

* Update files/en-us/learn/accessibility/html/index.md

* Update files/en-us/learn/accessibility/html/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* Tag syntax code blocks with js-nolint, part 6

Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/filereadersync/readasarraybuffer/index.md

* Update files/en-us/web/api/filesystemdirectoryhandle/entries/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* Tag syntax code blocks with js-nolint, part 7

Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/htmlvideoelement/msstereo3drendermode/index.md

* Update files/en-us/web/api/filesystemfilehandle/createwritable/index.md

* Update files/en-us/web/api/filesystemdirectoryhandle/resolve/index.md

* Update files/en-us/web/api/filesystemdirectoryhandle/values/index.md

* Update files/en-us/web/api/filesystemfilehandle/getfile/index.md

* Update files/en-us/web/api/filesystemhandle/querypermission/index.md

* Update files/en-us/web/api/htmlformelement/reportvalidity/index.md

* Update files/en-us/web/api/filesystemhandle/requestpermission/index.md

* Update files/en-us/web/api/filesystemwritablefilestream/seek/index.md

* Update files/en-us/web/api/filesystemwritablefilestream/truncate/index.md

* Update files/en-us/web/api/htmlcollection/nameditem/index.md

* Update files/en-us/web/api/htmlmediaelement/mscleareffects/index.md

* Update files/en-us/web/api/htmlvideoelement/mssetvideorectangle/index.md

* Update files/en-us/web/api/htmlvideoelement/msstereo3dpackingmode/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* Tag syntax code blocks with js-nolint, part 8

Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/ndefreader/write/index.md

* Update files/en-us/web/api/ndefrecord/ndefrecord/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* Tag syntax code blocks with js-nolint, part 9

Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/ndefrecord/torecords/index.md

* Update files/en-us/web/api/nodelist/entries/index.md

* Update files/en-us/web/api/nodelist/foreach/index.md

* Update files/en-us/web/api/nodelist/keys/index.md

* Update files/en-us/web/api/nodelist/values/index.md

* Update files/en-us/web/api/presentationrequest/start/index.md

* Update files/en-us/web/api/relativeorientationsensor/relativeorientationsensor/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* Tag syntax code blocks with js-nolint, part 10

Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/rtcicetransport/getremoteparameters/index.md

* Update files/en-us/web/api/rtcpeerconnection/addstream/index.md

* Update files/en-us/web/api/rtcpeerconnection/close/index.md

* Update files/en-us/web/api/rtcpeerconnection/getidentityassertion/index.md

* Update files/en-us/web/api/rtcpeerconnection/getreceivers/index.md

* Update files/en-us/web/api/rtcpeerconnection/getsenders/index.md

* Update files/en-us/web/api/rtcpeerconnection/removestream/index.md

* Update files/en-us/web/api/rtcpeerconnection/removetrack/index.md

* Update files/en-us/web/api/rtcrtpsender/replacetrack/index.md

* Update files/en-us/web/api/selection/setbaseandextent/index.md

* Update files/en-us/web/api/serviceworkercontainer/startmessages/index.md

* Update files/en-us/web/api/storagemanager/persist/index.md

* Update files/en-us/web/api/storagemanager/persisted/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* Tag syntax code blocks with js-nolint, part 11

Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/svgpoint/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* Tag syntax code blocks with js-nolint, part 12

Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/window/showdirectorypicker/index.md

* Update files/en-us/web/api/webgl_multi_draw/multidrawarraysinstancedwebgl/index.md

* Update files/en-us/web/api/webgl_multi_draw/multidrawarrayswebgl/index.md

* Update files/en-us/web/api/webgl_multi_draw/multidrawelementsinstancedwebgl/index.md

* Update files/en-us/web/api/webgl_multi_draw/multidrawelementswebgl/index.md

* Update files/en-us/web/api/webglrenderingcontext/attachshader/index.md

* Update files/en-us/web/api/webglrenderingcontext/detachshader/index.md

* Update files/en-us/web/api/window/showdirectorypicker/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* Tag syntax code blocks with js-nolint, part 13

Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/web/api/xrwebgllayer/antialias/index.md

* Update files/en-us/web/api/xrwebgllayer/getnativeframebufferscalefactor/index.md

* Update files/en-us/web/api/xrwebgllayer/getnativeframebufferscalefactor/index.md

* Update files/en-us/web/http/proxy_servers_and_tunneling/proxy_auto-configuration_pac_file/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
Adding to mdn#19177

- Updating syntax code blocks with the new tag. Refer: mdn/yari#7017
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* Tag `example-bad` blocks with html-nolint

- Updating example-bad code blocks with the new tag. Refer: mdn/yari#7017

* Update files/en-us/learn/accessibility/html/index.md

* Update files/en-us/learn/accessibility/html/index.md

* Removed suggested no nolints

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>

* remove a trailing space

* Update files/en-us/learn/accessibility/html/index.md

* Update files/en-us/learn/accessibility/html/index.md

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants