Esri / esri-leaflet-doc

Documentation, API Reference and Samples
https://esri.github.io/esri-leaflet/
Apache License 2.0
81 stars 1.12k forks source link

Add Linting #241

Closed jwasilgeo closed 5 years ago

jwasilgeo commented 5 years ago

esri-leaflet is wired up with semistandard linting (here, here), but I haven't yet identified that this repo enforces linting via an npm script or a Grunt task.

Since we're already using Grunt here, does anyone have a preference on the latest and greatest in that ecosystem? I admittedly haven't been staying up to date with that lately.

jwasilgeo commented 5 years ago

I dug a little deeper into the 🐇 🕳 and found that grunt-contrib-jshint is already in the npm packages devDependencies, but seems to be unused yet by Grunt.

https://github.com/Esri/esri-leaflet-doc/blob/master/package.json#L26

I'll attempt to wire it up into our Grunt tasks. I'll keep on researching what the best solution is these days in the Grunt ecosystem for both code correctness and consistent styling while introducing the least amount of changes (thus prettier might be a bit too much right now).

Implementing this approach:

jwasilgeo commented 5 years ago

Work is underway in branch 241-add-linting.

IMPORTANT NOTE: I have not consistently linted code snippets housed in <code> blocks in src/pages/api-reference .md files. If someone wants to figure that out as part of eslint, by all means.

So far eslint and the plugins I have wired up deal with actual .js files, or blocks of js within <script> tags in .hbs files, or blocks of js within markdown code blocks in .md files.

gavinr commented 5 years ago

@jwasilgeo re:

1.

find out why the grunt docs:build task calls 'sass' twice; can we insert 'eslint' anywhere here?

I think this is due to sass being listed twice here. @jgravois do you happen to know/remember if this was done on purpose or perhaps it's simply a mistaken duplication.


2.

decide if we want to ignore some remaining eslint errors in src/pages/tutorials .md files decide if we want to ignore some remaining eslint errors in src/pages/api-reference .md files

I think my changes here addressed these, correct?

jgravois commented 5 years ago

never attribute to malice what is adequately explained by john's carelessness or stupidity on john's part. 😇 - Hanlon's Razor (amended)

jwasilgeo commented 5 years ago
  1. @jgravois 😆😆; @gavinr shall we strike one of those sass listings and figure out where to add in eslint without its own error reporting forcing an early exit during the given grunt task(s)?

  2. Perfect, thanks again for that @gavinr. I'll check off those items in the todo list above.

jwasilgeo commented 5 years ago

The more I look at it the less sure I am about which sass to remove on this line. @jgravois @gavinr I might just leave it alone for right now.

jgravois commented 5 years ago

The more I look at it the less sure I am about which sass to remove.

I wouldn't expect it to matter which one you remove. are you seeing something weird locally?

jwasilgeo commented 5 years ago

Nah, I'm just tracking too many changes and subtopics in my mind and getting a little turned around. I'll strike one and commit the change. Thank you!

jwasilgeo commented 5 years ago

Closed via PR #244.