Skip to content

Adds eslint config extends with only whitespace rules enabled #1749

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 1, 2018
Merged

Adds eslint config extends with only whitespace rules enabled #1749

merged 1 commit into from
Mar 1, 2018

Conversation

sharmilajesupaul
Copy link
Contributor

@sharmilajesupaul sharmilajesupaul commented Feb 27, 2018 •
edited
Loading

Adds a change to pull in the eslint config from the project root, turn off all rules except the whitespace rules explicitly listed in the array.

This list is hardcoded for now, if there are any that I'm missing please let me know:

[
  'array-bracket-newline',
  'array-bracket-spacing',
  'array-element-newline',
  'arrow-spacing',
  'block-spacing',
  'comma-spacing',
  'computed-property-spacing',
  'dot-location',
  'eol-last',
  'func-call-spacing',
  'function-paren-newline',
  'generator-star-spacing',
  'implicit-arrow-linebreak',
  'indent',
  'key-spacing',
  'keyword-spacing',
  'line-comment-position',
  'linebreak-style',
  'multiline-ternary',
  'newline-per-chained-call',
  'no-irregular-whitespace',
  'no-mixed-spaces-and-tabs',
  'no-multi-spaces',
  'no-regex-spaces',
  'no-spaced-func',
  'no-trailing-spaces',
  'no-whitespace-before-property',
  'nonblock-statement-body-position',
  'object-curly-newline',
  'object-curly-spacing',
  'object-property-newline',
  'one-var-declaration-per-line',
  'operator-linebreak',
  'padded-blocks',
  'padding-line-between-statements',
  'rest-spread-spacing',
  'semi-spacing',
  'semi-style',
  'space-before-blocks',
  'space-before-function-paren',
  'space-in-parens',
  'space-infix-ops',
  'space-unary-ops',
  'spaced-comment',
  'switch-colon-spacing',
  'template-tag-spacing',
  'import/newline-after-import',
]

ljharb
ljharb requested changes Feb 27, 2018
View reviewed changes
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Can we also add this to the readmes?

packages/eslint-config-airbnb/whitespace.js Outdated
@@ -0,0 +1 @@
require('../eslint-config-airbnb-base/whitespace');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will need extra react-specific whitespace rules?

@sharmilajesupaul
Adds config entry point with only whitespace rules enabled
6373dab 
Adds a change to eslint-config-airbnb and eslint-config-airbnb-base to pull a list of rules from the project root and return a config with all rules turned off except the whitespace rules explicitly listed in the array. Also adds entry point data to readme.
@sharmilajesupaul
Copy link
Contributor Author

added info to readme @ljharb, here's what it looks like:

eslint-config-airbnb-base/whitespace

This entry point that only warns on whitespace rules and sets all other rules to warnings. View the list of whitespace rules here.

ljharb
ljharb approved these changes Mar 1, 2018
View reviewed changes
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great!

@ljharb ljharb merged commit 8e6363c into master Mar 1, 2018
@ljharb ljharb deleted the whitespace-rules branch March 1, 2018 03:02
1pete
1pete reviewed Mar 1, 2018
View reviewed changes
packages/eslint-config-airbnb-base/whitespace.js
const { CLIEngine } = require('eslint');

function onlyErrorOnRules(rulesToError, config) {
const errorsOnly = { ...config };
Copy link
Contributor

Choose a reason for hiding this comment

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

This object spread property in nodejs is introduced at version 8+. If this file is meant to be run with nodejs 8+, then we don't need to depend on more lib e.g. object.entries, Object.entries already exists there.

I can make a PR to clean this. What do you think, @ljharb?

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if we convert this file to be compatible with nodejs 4 which is declared in package.json, we still don't need this extra dependency, since eslint already depends on lodash, then we can add it too and use _.forEach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops, this should use object.assign instead of spread.

There’s no cost to an extra dev dep, and I’d prefer not to use lodash.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, then we need to change array destructuring at line 10 too.

I'll make a PR, ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure.

@1pete 1pete mentioned this pull request Mar 1, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@1pete 1pete 1pete left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sharmilajesupaul @ljharb @1pete