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

<!-- eslint-disable --> does not work in the v1.0.0-beta.6 #69

Closed
vsemozhetbyt opened this issue May 11, 2017 · 10 comments
Closed

<!-- eslint-disable --> does not work in the v1.0.0-beta.6 #69

vsemozhetbyt opened this issue May 11, 2017 · 10 comments
Labels

Comments

@vsemozhetbyt
Copy link

vsemozhetbyt commented May 11, 2017

1. I've found an error on the v1.0.0-beta.4: <!-- eslint-disable some-rule --> completely disables parsing the whole fragment and even the next fragment:

test.md

### Heading

Text

` ``js
console.log('a')

` ``

` ``js
console.log('b')

` ``

` ``js
console.log('c')

` ``

Error log:

   6:1   error  Use the global form of 'use strict'     strict
   6:17  error  Missing semicolon                       semi
  11:1   error  Use the global form of 'use strict'     strict
  11:17  error  Missing semicolon                       semi
  16:1   error  Use the global form of 'use strict'     strict
  16:17  error  Missing semicolon                       semi

test.md with comments:

### Heading

Text

<!-- eslint-disable strict -->
` ``js
console.log('a')

` ``

` ``js
console.log('b')

` ``

` ``js
console.log('c')

` ``

Error log (semi errors for the fragments 1 and 2 are not reported; fragment 2 is ignored):

  17:1   error  Use the global form of 'use strict'     strict
  17:17  error  Missing semicolon                       semi

2. So I've tried to update to v1.0.0-beta.6 and make a PR for Node.js. v1.0.0-beta.6 seems to fix this issue, but it has a new breaking one: code blocks with <!-- eslint-disable --> are parsed and throws if parsing errors found:
test.md:

### Heading

Text

<!-- eslint-disable -->
` ``js
console.log('a') bang

` ``

Error log:

  7:18  error  Parsing error: Unexpected token bang
@btmills
Copy link
Member

btmills commented May 12, 2017

@vsemozhetbyt thanks for reporting these!

  1. Disable comments fall through

    Yikes! That's not a good bug. Based on a quick bisect, it looks like it was fixed in d2f2962. Strangely, I had an empty section of tests where that should've been tested, but I guess I never wrote the tests. I've filed Chore: Add test ensuring config comments do not fall through #70, which adds tests to make sure config comments don't fall through to subsequent blocks.

  2. eslint-disable still reports parser errors

    This looks like it's behaving as intended, though maybe not as expected. This has come up a handlful of times in the eslint/eslint issue tracker, so we added a note to the docs about eslint-disable comments:

    Comments that disable warnings for a portion of a file tell ESLint not to report rule violations for the disabled code. ESLint still parses the entire file, however, so disabled code still needs to be syntactically valid JavaScript.

    While it would be technically feasible to override that behavior in the preprocessor, I'd like to keep this plugin inline with ESLint's intended behavior.

    What use case do you have where you have JS code fences (```js ... ```) that don't contain valid JS? Maybe there's a way to support it still. For example, this plugin only checks code fences that are explicitly marked as JS syntax. If the code isn't valid JS, are you able to change the code fence tag to something else?

@vsemozhetbyt
Copy link
Author

vsemozhetbyt commented May 12, 2017

@btmills
We have started to use eslint-plugin-markdown in Node.js CI and local builds from the nodejs/node#12563 with the v1.0.0-beta.4. We have many code blocks marked with the <!-- eslint-disable --> and they were not parsed in v1.0.0-beta.4, so we cannot update now without revisiting all these fragments.

Use cases examples for docs: REPL sessions logs that are not valid JS but need syntax highlighting; big Object listings that erroneously parsed as code blocks by ESLint (wrapping them in parentheses make them less readable). It would be very handy to have an option to disable eslint-plugin-markdown for such fragments: code fragments in docs do have some specificity.

@btmills
Copy link
Member

btmills commented May 13, 2017

@vsemozhetbyt that makes sense, thanks for explaining.

As a first step, I tracked down what was causing the behavior you observed in beta.4. It turns out that the Markdown parser in use at the time wasn't picking up code fences unless they were preceded by blank lines. I've added a test in #72 to make sure that doesn't happen again.

<!-- eslint-disable -->

```js
// This block is seen by both beta.4 and beta.6
```

<!-- eslint-disable -->
```js
// This block is invisible to beta.4 but seen by beta.6
```

Since your disable comments and code fences were on adjacent lines, the processor wasn't even getting those code blocks back from the parser. I'm glad that bug was fixed in a beta! 😅

Now, as for supporting your use case. I'd like to keep the meaning of eslint-disable the same in ESLint core and this processor, as I fear it could introduce confusion otherwise. With that in mind, how about I add support for a new <!-- eslint-skip --> comment that would exclude the code fence from parsing entirely? If that works for you, I'll run it by the rest of the team to make sure I'm not stepping on any toes with that. It would look like this:

<!-- eslint-skip -->
```js
!@#$%^&*() <-- Doesn't even get parsed. The processor ignores this block entirely.
```

<!-- eslint-disable -->
```js
alert('Hello, ' + "world!") // Only check for syntax errors, ignore rules
```

@vsemozhetbyt
Copy link
Author

vsemozhetbyt commented May 13, 2017

@btmills I like it) Let's see what others think.

@Trott
Copy link

Trott commented May 27, 2017

@btmills I like it) Let's see what others think.

Me too.

@refack
Copy link

refack commented May 28, 2017

With that in mind, how about I add support for a new

👍

@btmills
Copy link
Member

btmills commented Jun 17, 2017

I just submitted a pull request to implement <!-- eslint-skip --> comments in Markdown: #73. Let me know what you think!

@vsemozhetbyt
Copy link
Author

@btmills Thank you! Looking for landing and the new version to update the plugin in the Node.js.

@btmills btmills closed this as completed in f8ba18a Jul 2, 2017
@btmills
Copy link
Member

btmills commented Jul 2, 2017

<!-- eslint-skip --> comments have been released in v1.0.0-beta.7.

@vsemozhetbyt
Copy link
Author

@btmills Thank you!

Trott added a commit to Trott/io.js that referenced this issue Jul 3, 2017
An issue affecting Node.js source has been fixed in
eslint-plugin-markdown so we don't need to pin it to beta-4 anymore.

Refs: eslint/eslint-plugin-markdown#69
addaleax pushed a commit to nodejs/node that referenced this issue Jul 11, 2017
* Remove pinning of eslint-plugin-markdown

  An issue affecting Node.js source has been fixed in
  eslint-plugin-markdown so we don't need to pin it to beta-4 anymore.

  Refs: eslint/eslint-plugin-markdown#69

* Update eslint-plugin-markdown up to 1.0.0-beta.7

* Fix docs for eslint-plugin-markdown@1.0.0-beta.7

PR-URL: #14047
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to nodejs/node that referenced this issue Jul 18, 2017
* Remove pinning of eslint-plugin-markdown

  An issue affecting Node.js source has been fixed in
  eslint-plugin-markdown so we don't need to pin it to beta-4 anymore.

  Refs: eslint/eslint-plugin-markdown#69

* Update eslint-plugin-markdown up to 1.0.0-beta.7

* Fix docs for eslint-plugin-markdown@1.0.0-beta.7

PR-URL: #14047
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit to nodejs/node that referenced this issue Jul 19, 2017
* Remove pinning of eslint-plugin-markdown

  An issue affecting Node.js source has been fixed in
  eslint-plugin-markdown so we don't need to pin it to beta-4 anymore.

  Refs: eslint/eslint-plugin-markdown#69

* Update eslint-plugin-markdown up to 1.0.0-beta.7

* Fix docs for eslint-plugin-markdown@1.0.0-beta.7

PR-URL: #14047
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Trott added a commit to Trott/io.js that referenced this issue Feb 28, 2019
eslint-plugin-markdown is being installed @next to get a bugfix for
eslint/eslint-plugin-markdown#69 but that
bugfix is in 1.0.0. Go back to installing @latest rather than @next.
Trott added a commit to Trott/io.js that referenced this issue Mar 2, 2019
eslint-plugin-markdown is being installed @next to get a bugfix for
eslint/eslint-plugin-markdown#69 but that
bugfix is in 1.0.0. Go back to installing @latest rather than @next.

PR-URL: nodejs#26345
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to nodejs/node that referenced this issue Mar 2, 2019
eslint-plugin-markdown is being installed @next to get a bugfix for
eslint/eslint-plugin-markdown#69 but that
bugfix is in 1.0.0. Go back to installing @latest rather than @next.

PR-URL: #26345
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants