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

Linting tweaks and updates #81

Merged
merged 2 commits into from
Mar 3, 2020
Merged

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Feb 29, 2020

  • Breaking change: Bump to Node 4.0 (required by estraverse)
  • Refactoring: remove now unneeded Array.isArray polyfill
  • npm: Update estraverse dep. minor version and devDeps.
  • Linting (ESLint): Add node_modules and dist to ignore file and unignore .eslintrc.js;
    use "js" extension (besides succincter syntax, allows comments); enforce semi
    rule; lint parser.js (disabling some rules) to catch any buggy generator issues
  • Linting (package.json): Add recommended fields (contributors, bugs, homepage);
    update repository URL
  • Refactoring: Fix broken or redundant HTML
  • Maintenance: Enforce 2 sp. for JSON in .editorconfig (per current package.json)
  • npm: Add build scripts (leading currently to Makefile)
  • npm: Update estraverse dep. minor version and devDeps.
  • .travis.yml: Slightly simpler syntax

…unignore `.eslintrc.js`;

    use "js" extension for comments; enforce `semi` rule; lint `parser.js` to catch any buggy generator issues
- Linting (package.json): Add recommended fields (contributors, bugs, homepage); update repository URL
- Refactoring: Fix broken or redundant HTML
- Maintenance: Enforce 2 sp. for JSON in `.editorconfig` (per current `package.json`)
- npm: Add build scripts (leading currently to Makefile)
@brettz9 brettz9 changed the title Linting tweaks Linting tweaks and updates Feb 29, 2020
@brettz9
Copy link
Contributor Author

brettz9 commented Feb 29, 2020

Btw, previous to my HTML tweaks, the runner seems broken anyways, as it only indicates 0/0 tests running.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@michaelficarra
Copy link
Member

I think it's about time that we drop the old node support, but it would've been nice to call that out in the PR description.

.eslintrc.js Outdated
overrides: [{
files: 'parser.js',
rules: {
// We could disable checking as auto-generated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes just ignore the generated file(s).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sure. I personally like to check, albeit without applying mere formatting rules, since one doesn't know if a dependency did its own linting, especially in such a case as this where the code to lint was generated at our calling, rather than being bundled with the dependency. But it was safe when checked. :)

IMHO, linting normal dependencies is also a good idea (though I haven't done that here no less because eslint does not, unforutnately imo, allow linting of node_modules even with ignore overrides). I think this would be even more compelling of a use case than checking one's own code to find not only bugs but unwanted privileged module/API use if the linting is robust enough (e.g., preventing or checking dynamic access to require, setting of global, etc.), but again this is not supported anyways.

- Refactoring: remove now unneeded `Array.isArray` polyfill
- npm: Update estraverse dep. minor version and devDeps.
@brettz9
Copy link
Contributor Author

brettz9 commented Mar 3, 2020

My apologies re: the breaking change not having been listed. I think I originally just thought I was tacking on some minor changes, especially with the estraverse change having been only a minor version bump. But on seeing its minimum requirements, I saw we could update and get rid of the Array.isArray polyfill as well, so amended accordingly.

I've now added it to the main description as follows, along with another line item not previously mentioned (also added later because of the update):

  • Breaking change: Bump to Node 4.0 (required by estraverse)
  • Refactoring: remove now unneeded Array.isArray polyfill

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants