[eslint] Enforce @ignore before comment

[mbrookes]

• PR has tests / docs demo, and is linted.
• Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
• Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Adds a custom eslint rule to enforce placing @ignore at the start of the comment block.

Resolves: #3589 (comment)

[alitaheri]

WOW O.O That was fast 👍 👍 👍 Thanks 😍

@callemall/material-ui Take a look and merge, somebody else 😆

[oliviertassinari]

Could we enforce it with 2?

[oliviertassinari]

My sublime text plugin is having trouble finding the definition of this rule. Is this only me?

[alitaheri]

@oliviertassinari It's a custom rule, I don't think IDEs support them. Besides the build is passing, so I'm pretty sure it's IDE related.

[nathanmarks]

@alitaheri @oliviertassinari

I see the same thing. This is making the top line of every file show a warning in my IDE 👎

src/DropDownMenu/DropDownMenu.jsx: line 1, col 1, Warning - Definition for rule 'require-@ignore-before-comment' was not found (require-@ignore-before-comment)

[nathanmarks]

This does not work with eslint either. It is not an IDE issue:

PoweredbySearch on Powereds-MacBook-Pro-2.local in ~/Development/repos/nathanmarks/material-ui(23h32m|pr/3611)
$ node_modules/.bin/eslint src

/Users/PoweredbySearch/Development/repos/nathanmarks/material-ui/src/card/index.js
  1:1  warning  Definition for rule 'require-@ignore-before-comment' was not found  require-@ignore-before-comment

/Users/PoweredbySearch/Development/repos/nathanmarks/material-ui/src/date-picker/index.js
  1:1  warning  Definition for rule 'require-@ignore-before-comment' was not found  require-@ignore-before-comment

/Users/PoweredbySearch/Development/repos/nathanmarks/material-ui/src/DropDownMenu/index.js
  1:1  warning  Definition for rule 'require-@ignore-before-comment' was not found  require-@ignore-before-comment

/Users/PoweredbySearch/Development/repos/nathanmarks/material-ui/src/grid-list/index.js
  1:1  warning  Definition for rule 'require-@ignore-before-comment' was not found  require-@ignore-before-comment

/Users/PoweredbySearch/Development/repos/nathanmarks/material-ui/src/hoc/selectable-enhance.js
  1:1  warning  Definition for rule 'require-@ignore-before-comment' was not found  require-@ignore-before-comment

/Users/PoweredbySearch/Development/repos/nathanmarks/material-ui/src/index.js
  1:1  warning  Definition for rule 'require-@ignore-before-comment' was not found  require-@ignore-before-comment

# it keeps going ;)

... etc

[nathanmarks]

@mbrookes @alitaheri

It looks like the current implementation is tied to the gulp config. Can this be setup so that we can run eslint without gulp and have the rule run?

Ideally we'd be able to just run node_modules/.bin/eslint src (without args). Does this mean having the rule setup as some sort of plugin for eslint instead of a script in the repo?

[alitaheri]

@mbrookes rebase please 😅

[nathanmarks]

@alitaheri this cannot be merged as is, it was gulp-only the way it was configured. That's why I added "Need Revision".

I think it should be implemented as a plugin.

We could use --rulesdir, but I think it would be nicer to have it as a plugin outside of the repo. eslint looks like it has an easy plugin system to drop this into: http://eslint.org/docs/developer-guide/working-with-plugins @mbrookes

[alitaheri]

but I think it would be nicer to have it as a plugin outside of the repo,

Yeah I agree 👍 👍 @mbrookes Maybe you can extend it a bit and make it a general purpose rule of it own. I'm sure many can benefit from it 👍

[mbrookes]

Oddly eslint doesn't support rulesDir in .eslintrc - the one place you'd really want it (which is why I went the gulp route to try to avoid an external dependancy).

As we've dropped gulp, and to make this work effectively across editors, it requires a custom plugin: https://github.com/mbrookes/eslint-plugin-material-ui

We can add additional rules to that as needed.

[oliviertassinari]

What about publishing the eslint rules package for Material-UI from this repository?

[mbrookes]

That would be better. Shall I hang it off root?

[oliviertassinari]

Where should we put the folder for this new npm package? That's the hardest part.

We could follow https://github.com/facebook/react/tree/master/packages with /packages/eslint-plugin-material-ui/ and as suggested by #3670, having a script that move what is needed under /packages/material-ui/, and later /packages/material-ui-native/.

What do you think?

[mbrookes]

Well I wouldn't advocate rearranging the directory structure just to accommodate this small package, especially as it is only a dev dependancy, rather than a package to be published.

I had put it in / before I read your suggestion, and it's working fine, although I may have uncovered a Travis bug with locally installed npm packages. (I changed package.json to install eslint-plugin-material-ui from the local directory rather than npm, which works here, but not, apparently, on Travis.)

[nathanmarks]

@mbrookes there's some great discussions going on in issue #3670 RE folder structure

[mbrookes]

@nathanmarks - yes, it was based on a discussion @alitaheri and I had earlier. But as I said above, this is only a dev dependancy, rather than a package to be published (assuming we can get past the Travis issue without resorting to publishing to npm), but even if not, it's still not a for-public-consumption package, so belongs within material-ui rather than along side it.

That is to say, even if material-ui goes into packages (an unnecessary layer of nesting IMHO, but I'll save that for the other discussion), this package would still reside within it.

[mbrookes]

Thanks @oliviertassinari, it looks like the problem was the blanket exclusion of lib in .gitignore whic h meant the important part of the package wasn't included in the commit!

This .gitignore rule would also have caused us problems with @newoga's solution to the directory reorg name-clash issue: #2679 (comment)

I've made it /lib instead, and now all is good.

[nathanmarks]

@mbrookes Think this comment section needs a cleanup 👍

[mbrookes]

Uh, whoops! 😲

[nathanmarks]

@oliviertassinari @mbrookes are we happy with this sitting in the root for now? Or should it go under a subfolder?

[oliviertassinari]

That's fine for me under the root folder for now.

[nathanmarks]

Do we need these since the root repo has both mocha and eslint?

[nathanmarks]

@oliviertassinari FYI, the /packages thing is very widespread! eslint do it for their own rule plugin too: https://github.com/eslint/eslint/tree/master/packages/eslint-config-eslint

[mbrookes]

I can move it to packages to get the ball rolling if that's the direction we're taking, or we can merge as is and move it later. Let me know.

[nathanmarks]

@mbrookes did you just rebase? what about also adding [Core] to the last 2 commit messages?

[mbrookes]

I'm assuming the commit's will get squashed, and I'll update the commit message then - not too fussed about the ones that will be deleted.

Since all of @oliviertassinari's eslint updates have [eslint] as the prefix, so I went with the for the PR title.

[nathanmarks]

@mbrookes Ahhh sorry, I thought you just rebased/squashed because everything was outdated!

[mbrookes]

No, you're right, I rebased, then noticed that the Stepper @ignores were already fixed, so dropped that commit and force pushed.

But haven't squashed yet... will do that if all looks good.

[nathanmarks]

typo in pluigin

[mbrookes]

I knew I should have cut & pasted that! And I need 👓 .

[oliviertassinari]

That looks good to me 👍.

[mbrookes]

Thanks, that's good enough for me. 😄