Page MenuHomePhabricator

Replace use of .eslintignore with ignore in Gruntfile.js in all repos, for consistency
Closed, DeclinedPublic

Description

The following skins and extensions have a .eslintignore file, but it also possible to exclude the vendor and node_modules folder over grunt config in Gruntfile.js

I would say it is better to use Gruntfile.js to see the ignore at one place, because jsonlint or other build tools are using it too.
Pro argument is that IDE can use the file (I have no idea, if there is any IDE support for eslint)

It is okay to use eslintignore or should we decide to use Gruntfile.js? That would remove a file from the root path and define a default handling.

Thanks for comments

This could also be done for .jshintignore or other tools over grunt.

Skins
  • Timeless
Extensions
  • ArticlePlaceholder
  • PropertySuggester
  • Wikibase
  • WikibaseLexeme
  • WikibaseMediaInfo
  • WikibaseQuality
  • WikibaseQualityConstraints
  • WikibaseQualityExternalValidation
  • Wikidata.org
  • WikimediaBadges

libs

  • data-values/value-view - gerrit
  • wikibase/javascript-api - gerrit
  • wikibase-serialization - rWBSERJS
  • datavalues-javascript - rDVJS
  • wikibase-data-model - rWBDMJS

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 27 2017, 5:26 PM

The pro argument is (like you said already): all the exclusions can be found in one file (as long as it integrates with grunt). That would mean, that it would be easier to see, what files will be checked with the tool (you do not need to open Gruntfile.js to see what is _included_ and then look for a specialized file in the projects root to see what is _excluded_).

The contra argument I can think of: One could assume, that excludes are defined in a separate ignore file, if no ignore file is present, they could assume, that no files are ignored. However, this argument can most likely be fixed when we agree on using Gruntfile.js and after some time it will be something that is "well-known", I think.

However, personally I do not have a strong opinion about what way to use, even if I kind of like to do not have so much .*ignore files in the projects root :) That are my 2 cents :P

thiemowmde triaged this task as Low priority.Oct 28 2017, 2:26 PM
thiemowmde added a subscriber: WMDE-leszek.

I vote for removing the separate .eslintignore files for the given reasons. Personally, I prefer having less clutter in the root directories of these projects. I wish the exclusions are closer to the rest of the configuration, either in the .eslintrc.json file (if this is possible), or in Gruntfile.js, as discussed.

I would love to hear @WMDE-leszek's opinion. If he agrees (or does not care) I would say go for it. I will then merge such patches.

So my two cents below. Please keep in mind that I don't have much experience in JS development, so I might be saying something which is complete rubbish.

  • As far as I understand, using .eslintignore file is a standard and kind of "native" way of defining paths to ignore used by eslint. It is also commonly in numerous JS projects I've seen around.
  • I am not entirely convinced that having a part of eslint config in .eslintrc.json, and then part (ignore paths) in grunt file is actually making things more clear.
  • Moving this to gruntfile would be "binding" (part of) esling config to a particular task runner. If developers of an extension, or WMF as a whole, decided to move to another JS task runner, all this information would also need to migrated etc. Not that it something which would be a rocket science, it is just making configuration stored in two different places and formats (as said in the point above).
  • I can totally understand preferring to have ignore path defined in a single place. On the other hand, I could also imagine the gruntfile might become cluttered if all kind of stuff is moved in there. It might be harder to find the relevant important information if there all kind of minor config also mashed in.

Overall: I have no strong preference on either option. Also, it is clear to me, all "arguments" made by me are really minor stuff. That said, from my perspective I also don't see a reason to "enforce" one or either way on extension/skin authors. This kind of thing seem to me to be something each extension devs could do whatever way they prefer, as long as both way are actually quite "standard" and nothing super weird that would be confusing to outside folks. I wouldn't oppose any decision, though, if this was something which is generally preferred :)

Change 427997 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/ArticlePlaceholder@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/427997

Change 428005 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/PropertySuggester@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428005

Change 428009 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/Wikidata.org@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428009

Change 428011 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/WikimediaBadges@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428011

Change 428012 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/WikibaseQualityExternalValidation@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428012

Change 428013 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/WikibaseQualityConstraints@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428013

Change 428014 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/WikibaseQuality@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428014

Change 428015 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/WikibaseMediaInfo@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428015

Change 428016 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/Wikibase@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428016

Change 428018 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/WikibaseLexeme@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428018

Change 428005 merged by jenkins-bot:
[mediawiki/extensions/PropertySuggester@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428005

Change 428009 merged by jenkins-bot:
[mediawiki/extensions/Wikidata.org@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428009

Change 427997 merged by jenkins-bot:
[mediawiki/extensions/ArticlePlaceholder@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/427997

Reedy removed a subscriber: Reedy.Apr 21 2018, 10:41 AM

Change 428061 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/skins/Timeless@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428061

Change 428061 merged by jenkins-bot:
[mediawiki/skins/Timeless@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428061

Jdforrester-WMF updated the task description. (Show Details)

Change 428015 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428015

Change 428018 merged by jenkins-bot:
[mediawiki/extensions/WikibaseLexeme@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428018

Change 428011 merged by jenkins-bot:
[mediawiki/extensions/WikimediaBadges@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428011

Change 428014 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQuality@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428014

Change 428013 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428013

Change 428016 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428016

Jdforrester-WMF renamed this task from Decide whether we want to use .eslintignore or ignore over GruntFile.js to Replace use of .eslintignore with ignore in Gruntfile.js in all repos, for consistency.Apr 23 2018, 7:13 PM

Change 428012 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityExternalValidation@master] Move information from .eslintignore into Gruntfile.js

https://gerrit.wikimedia.org/r/428012

Krinkle changed the task status from Open to Stalled.Sep 13 2018, 7:43 PM

I think moving to Gruntfile is a viable option, but it there are other options. If we really want to defocus on tooling integration, we should move eslintrc to Gruntfile as well. Or, keep the integration and keep both config+ignores out of Gruntfile (either in .eslint* files, or in package.json).

See T204176 for details.

Jdforrester-WMF closed this task as Declined.Apr 9 2020, 6:39 PM