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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint JavaScript snippets in the documentation #480

Open
wants to merge 28 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@papandreou
Copy link
Member

papandreou commented May 27, 2018

I tried eslint-plugin-markdown on https://github.com/unexpectedjs/unexpected/tree/feature/lint-markdown but ran into some challenges:

  • The plugin skips past the fenced blocks that use the js#async:true and js#evaluate:false syntax. This is fixable, but probably not upstreamable, as it appears that this syntax is something we made up :) Fixed by following Sune's suggestion in unexpectedjs/unexpected-markdown#32

  • The plugin doesn't seem to do anything when using eslint --fix. This is probably also fixable if we dig into it. This problem is fixed in the latest version of eslint-plugin-markdown! 馃帀

  • The plugin doesn't carry over the context from previous blocks, so it complains that 'add' is defined but never used in the first block and that 'add' is not defined in the second block here:

    ```js#evaluate:false
    function add (a, b) { return a + b; };
    ```
    
    Our test file would look like this:
    
    ```js#evaluate:false
    describe('math.js', function () {
      describe('add', function () {
        it('is a function', function () {
          expect(add, 'to be a', 'function');
        });
    
        it('adds numbers', function () {
          expect(add(1, 3), 'to be', 4);
        });
      });
    });
    ```

    An easy way to avoid that could be to disable the no-unused-vars and no-undef rules when processing markdown, but that limits the utility a bit. Fixed by disabling those two rules. The code snippets are also executed, that should be enough anyway

@papandreou papandreou added the WIP label May 27, 2018

@papandreou papandreou changed the title Lint JavaScript snippets in the documation Lint JavaScript snippets in the documentation Jun 19, 2018

@sunesimonsen

This comment has been minimized.

Copy link
Member

sunesimonsen commented Jun 30, 2018

I was initially just thinking about running prettier on the documentation files. I'm doing that manually in chance-generators with good results. Linting would of cause be great, but it is also a harder problem.

About the custom language syntax for the code blocks, maybe we should consider also supporting a comment before the block:

<!-- evaluate:false -->
```js
console.log('wat')
```

This will allow prettier to format the block and the comment will not be shown.

@papandreou

This comment has been minimized.

Copy link
Member

papandreou commented Jun 30, 2018

Good point about the half victory of at least getting the reformatting to work.

I was initially just thinking about running prettier on the documentation files. I'm doing that manually in chance-generators with good results. Linting would of cause be great, but it is also a harder problem.

Already did that (also manually) in #479 so the snippets are already "pretty" :)

Good idea about putting the #evaluate:false (and #async:true) modifiers in a separate HTML comment. Seems like that would help with interoperability in either scenario.

@sunesimonsen

This comment has been minimized.

Copy link
Member

sunesimonsen commented Jun 30, 2018

I added an issue for it on unexpected-markdown unexpectedjs/unexpected-markdown#12

@papandreou papandreou force-pushed the feature/lint-markdown branch from 39e8b45 to c78fb3f Jan 10, 2019

papandreou added some commits Jan 10, 2019

Disable the no-unused-vars and no-undef rules
Avoid errors from the snippets being linted independently
Convert ```js#flags to preceding HTML comments
perl -pi -e 'BEGIN{undef $/;} s/```js#async:true/<!-- async:true -->\n```js/g;' `find documentation -name '*.md'`
perl -pi -e 'BEGIN{undef $/;} s/```js#evaluate:false/<!-- evaluate:false -->\n```js/g;' `find documentation -name '*.md'`
perl -pi -e 'BEGIN{undef $/;} s/```js#freshExpect:true/<!-- freshExpect:true -->\n```js/g;' `find documentation -name '*.md'`
perl -pi -e 'BEGIN{undef $/;} s/```js#skipBrowser:true/<!-- skipBrowser:true -->\n```js/g;' `find documentation -name '*.md'`
@papandreou

This comment has been minimized.

Copy link
Member

papandreou commented Jan 10, 2019

Picked this up again and made some progress. Now npm run lint (and eslint --fix --ext js,md) also processes the js snippets in the documentation.

There're some errors we need to sort out: https://travis-ci.org/unexpectedjs/unexpected/jobs/478065429#L489-L614

It's mostly top level return in async snippets and some snippets that aren't valid JavaScript and are mostly for illustrative purposes. Unfortunately eslint-plugin-markdown doesn't understand our <!-- evaluate: false --> syntax, so it tries to parse them anyway, for example this one:

```js#evaluate:false
expect.addAssertion('<array> [not] to be (sorted|ordered) <function?>', function(expect, [3,2,1], reverse){
expect([3,2,1], '[not] to equal', [].concat([3,2,1]).sort(reverse));
});
```

... But come to think of it, maybe for this exact case we could exploit that it doesn't recognize js#evaluate:false as JavaScript snippets at all 馃樇

@papandreou

This comment has been minimized.

Copy link
Member

papandreou commented Jan 12, 2019

I think this is in a good state now. The only remaining obstacle is those 5 snippets that aren't valid JavaScript:

The ideal scenario would be for both unexpected-markdown and eslint-plugin-markdown to skip them, while still syntax highlighting them.

We can either:

  1. Leave it like it is now, which is a bit hacky because we rely the old ```js#evaluate:false syntax not being recognized as JavaScript by eslint-plugin-markdown.
  2. Stop pretending they're JavaScript by fencing them with ``` instead of ```js. The downside is that then those 5 snippets wouldn't be syntax highlighted.
  3. Try to make them valid JavaScript.
  4. Get [the <!-- eslint-skip --> syntax]https://github.com/eslint/eslint-plugin-markdown#skipping-blocks) to interop with unexpected-markdown's new flag passing via a preceding HTML comment. Both only work when they're the only HTML comment on the immediately preceding line, and the eslint-plugin-markdown one doesn't tolerate our flags being present in their comment. We could fix it by allowing our HTML comment to be on the line before an <!-- eslint... ---> one, but that also starts to feel too hacky.

I think I prefer 1) or 2), combined with 3) for those of the snippets that maybe could be valid JavaScript.

@papandreou papandreou removed the WIP label Jan 12, 2019

@papandreou papandreou requested a review from sunesimonsen Jan 12, 2019

Merge branch 'master' into feature/lint-markdown
* master:
  Updated the changelog
  11.0.1
  Build unexpected.js
  Remove ad hoc disabling of the no-unexpected-multiline eslint rule
  Whoops, two of those had to be Buffer.alloc
  Fix lint
  new Buffer => Buffer.from
  Add mocha env to the eslint config
  eslint --fix .
  Add standard and prettier/standard
  Replace eslint-config-pretty-standard with eslint-config-prettier
  Disable no-unexpected-multiline so standard and prettier don't battle each other
  eslint --fix .
  Fix expect.promise#inspect tests
  Avoid deprecation warnings in node.js 10 because we have methods called inspect

papandreou added some commits Jan 15, 2019

Merge branch 'master' into feature/lint-markdown
* master:
  .eslintrc.js: Remove unnecessary parserOptions
@papandreou

This comment has been minimized.

Copy link
Member

papandreou commented Jan 15, 2019

It's a bit annoying that our HTML comments cannot co-exist with the <!-- eslint ... --> ones because both demand to be the last HTML comment before the code block to take effect

Maybe we should expand unexpected-markdown to support passing ours before the the <!-- eslint ... --> ones, and also require some kind of "unexpected-markdown" marker in there so we don't get too confused.

Edit: One reason why it's annoying is that the <!-- eslint ... --> comments are handy for disabling certain eslint rules per block without an ugly // eslint-disable ... comment in the snippet itself.

@sunesimonsen

This comment has been minimized.

Copy link
Member

sunesimonsen commented Jan 16, 2019

Sounds like a plan

@papandreou papandreou force-pushed the feature/lint-markdown branch from 0430187 to 0761da9 Jan 17, 2019

papandreou added some commits Jan 17, 2019

Regain syntax highlighting of ```js blocks that don't parse
Now that we can combine eslint and unexpected-markdown directives:

<!-- unexpected-markdown evaluate:false -->
<!-- eslint-skip -->
@papandreou

This comment has been minimized.

Copy link
Member

papandreou commented Jan 18, 2019

@sunesimonsen, this is ready for a review now 馃

@@ -15,7 +15,7 @@
},
"scripts": {
"test": "make test && make test-chrome-headless",
"lint": "eslint .",
"lint": "eslint . && eslint --ext md documentation",

This comment has been minimized.

@papandreou

papandreou Jan 18, 2019

Member

Unfortunately the --ext switch is the only way to specify file extensions that eslint should process besides .js, so we can't move this to .eslintrc.js or upstream it to eslint-plugin-markdown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment