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

Switching to ESLint #43

Merged
merged 3 commits into from
Oct 25, 2015
Merged

Switching to ESLint #43

merged 3 commits into from
Oct 25, 2015

Conversation

pdehaan
Copy link
Contributor

@pdehaan pdehaan commented Oct 20, 2015

No description provided.

"validateLineBreaks": "LF",
"validateQuoteMarks": "'"
"preset": "node-style-guide",
"maximumLineLength": null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Config copied from gulpjs/gulp repo.

Copy link
Member

Choose a reason for hiding this comment

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

what are your thoughts on jscs? It seems like they aren't keeping up with the pace of eslint and eslint has a lot of style rules now. I wonder if we can drop jscs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm somewhat indifferent. I usually find a lot of overlap between JSCS and ESLint, which can lead to confusion if you dare specify different indenting rules in multiple places. So I guess my only concern here would be if we use a node-style-guide JSCS preset, but then change our ESLint rules to use XO (which may have some conflicts).

Although we can always extend the node-style-guide JSCS preset, override the conflicting rules after migrating to XO, and then republishing our own custom jscs-preset-gulp module once everything is green and passing tests.

Or, if you want to dump JSCS, we can use something like polyjuice to convert the .jscs config file into .eslintrc to see where the overlapping rules are, and then see if "eslint:recommended" or "xo" already define those rules (or conflicting rules).

Copy link
Member

Choose a reason for hiding this comment

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

ooooo that polyjuice idea is really cool. Could you give that a try?

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, actually, polyjuice isn't very interesting here since I guess it doesn't recursively parse "preset" fields.

$ polyjuice --jscs .jscsrc
{
  "rules": {
    "max-len": 0
  }
}

But here's an ESLint-ified version of the JSCS "node-style-guide" preset:

{
  "rules": {
    "brace-style": 2,
    "camelcase": [2, {"properties": "never"}],
    "comma-dangle": [2, "always-multiline"],
    "curly": [2, "all"],
    "indent": [2, 2, {"SwitchCase": 1}],
    "key-spacing": [2, {"beforeColon": false, "afterColon": true}],
    "linebreak-style": [2, "unix"],
    "lines-around-comment": [2, {"afterBlockComment": true, "afterLineComment": true}],
    "max-len": [2, 80], // We override this one...
    "new-cap": 2,
    "no-mixed-spaces-and-tabs": 2,
    "no-trailing-spaces": 2,
    "no-with": 2,
    "one-var": [2, {"uninitialized": "always", "initialized": "never"}],
    "quote-props": [2, "as-needed"],
    "quotes": [2, "single"],
    "space-after-keywords": [2, "always"],
    "space-before-blocks": [2, "always"],
    "space-before-function-paren": [2, "never"],
    "space-in-parens": [2, "never"],
    "space-infix-ops": 2,
    "space-return-throw-case": 0,
    "space-unary-ops": 2
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Cool, is there an easy way to diff this with XO's config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I forgot that polyjuice has a flag to report which rules have been discarded (the appropriately named --discarded or -d):

So here's what polyjuice can't convert from .jscsrc to .eslintrc rules:

$ polyjuice --jscs .jscsrc -d

disallowNewlineBeforeBlockStatements
requireCapitalizedComments
requireSpaceBeforeBinaryOperators
requireSpacesInFunction
requireBlocksOnNewline

So I say if those 5 rules are important to you, we keep JSCS in the workflow (and publish a custom jscs-preset-gulp preset with those 5 rules; so we can reuse them between repos).

If those 5 rules aren't important, let's remove the .jscsrc files and dependencies from the repos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not an easy way since XO's config is about 158 lines long.
But here's a mediocre DIFF between the current "node-style-guide" JSCS file and the defaults from XO:

  rules: {
    'brace-style': [2, '1tbs', {allowSingleLine: false}],
    'camelcase': [2, {properties: 'always'}],
    'comma-dangle': [2, 'never'],
    'curly': 2,
    'indent': [2, 2, {SwitchCase: 1}], // SAME
    'key-spacing': [2, {beforeColon: false, afterColon: true}], // SAME
    'linebreak-style': [2, 'unix'], // SAME
    // MISSING: `lines-around-comment`
    'new-cap': [2, {newIsCap: true, capIsNew: true}],
    'no-mixed-spaces-and-tabs': 2, // SAME
    'no-trailing-spaces': 2, // SAME
    'no-with': 2, // SAME
    'one-var': [2, 'never'],
    'quote-props': [2, 'consistent-as-needed'],
    'quotes': [2, 'single'], // SAME
    'space-after-keywords': [2, 'always'], // SAME
    'space-before-blocks': [2, 'always'], // SAME
    'space-before-function-paren': [2, {anonymous: 'always', named: 'never'}],
    'space-in-parens': [2, 'never'], // SAME
    'space-infix-ops': 2, // SAME
    'space-return-throw-case': 2,
    'space-unary-ops': 2, // SAME

So about 12 of the rules are the same, and the rest may need some code tweaking.

Copy link
Member

Choose a reason for hiding this comment

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

@pdehaan started looking at the 5 rules and the first one was something I don't want to lose. I've created a jscs-preset-gulp repo and added you as a collaborator.

@phated
Copy link
Member

phated commented Oct 21, 2015

@pdehaan I've published the jscs and eslint rules at 1.0

@phated
Copy link
Member

phated commented Oct 23, 2015

Looks like the merged jscs PR is conflicting with this one?

@pdehaan
Copy link
Contributor Author

pdehaan commented Oct 25, 2015

Rebased.

@phated
Copy link
Member

phated commented Oct 25, 2015

@pdehaan globbing for jscs on windows seems to be broken, any ways to avoid that?

@pdehaan
Copy link
Contributor Author

pdehaan commented Oct 25, 2015

Sorry, I don't use Windows, not sure why JSCS globbing would be broken there.
Was it passing before and something in my PR broke it?

phated added a commit that referenced this pull request Oct 25, 2015
@phated phated merged commit edc9a90 into gulpjs:master Oct 25, 2015
phated added a commit that referenced this pull request Dec 21, 2017
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.

None yet

2 participants