Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Sync with latest changes from liferay/clay (#25) #30

Merged
merged 3 commits into from
Mar 4, 2019
Merged

Conversation

wincent
Copy link
Contributor

@wincent wincent commented Mar 4, 2019

Sync with latest changes from liferay/clay (#25)

Based on looking at the current "develop" plugins and rules:

https://github.com/liferay/clay/blob/01432f23daa1a3abac3380b17b0545779bdefb1e/.eslintrc

Comments added inline:

"plugins": [
        // Already included:
        "notice",

        // The intent of this one is to avoid needing
        // a Symbol.iterator polyfill for older browsers.
        // This one is probably a bit too opinionated to
        // go in the default, but we are using it in multiple
        // places and IE doesn't have Symbol.iterator...
        "no-for-of-loops",

        // Probably don't want to include this in the default
        // because not all of our projects use React.
        "react"
],
"rules": {
        // See above.
        "no-for-of-loops/no-for-of-loops": 2,

        // On by default in eslint:recommended but we override the
        // default options.
        // "ignoreRestSiblings" strikes me as unnecessarily lax.
        // I'd rather see us use options like: `{argsIgnorePattern: '^_'}`.
        "no-unused-vars": ["error", {"ignoreRestSiblings": true}],

        // See above.
        "react/jsx-uses-react": "error",
        "react/jsx-uses-vars": "error",

        // Shouldn't be needed as Prettier should handle this.
        "no-mixed-spaces-and-tabs": ["error", "smart-tabs"],

        // See above.
        "notice/notice": [
                "error",
                {
                        "templateFile": "copyright.js"
                }
        ]
}

And in the PR:

https://github.com/liferay/clay/pull/1548/files#diff-df39304d828831c44a2b9f38cd45289c

"plugins": [
        // As above.
        "notice",
        "no-for-of-loops",
        "react",

        // Another React-related one that probably doesn't
        // belong in the base.
        "react-hooks",

        // This one seems fair enough.
        "sort-destructure-keys",

        // This one is a bit scary/invasive, and we haven't
        // used it outside of a PR yet, but we can try it;
        // if it works well then we should bake it in to
        // the default.
        "sort-imports-es6-autofix"
],
"rules": {
        // Should be handled by Prettier.
        "comma-dangle": [2, "always"],

        // This is a good one, and not in the default eslint:recommended.
        "default-case": 2,

        // Prettier.
        "eol-last": [2, "never"],

        // Prettier, and not on by default anyway.
        "max-len": 0,

        // Already in eslint:recommended.
        "no-console": 2,

        // See above.
        "no-for-of-loops/no-for-of-loops": 2,

        // Prettier.
        "no-mixed-spaces-and-tabs": ["error", "smart-tabs"],

        // This is a good one, and not in the default eslint:recommended.
        "no-return-assign": [2, "always"],

        // Already in eslint:recommended.
        "no-undef": 2,

        // This one is dubious, and not in default eslint:recommended.
        // (Dubious because allowing underscores after this but prohibiting
        // them in method names seems a bit arbitrary or contradictory).
        // I wouldn't put this one in the default.
        "no-underscore-dangle": [
                2,
                {
                        "allowAfterThis": true,
                        "enforceInMethodNames": true
                }
        ],

        // Note this contradicts the setting on "develop"
        // (which uses `ignoreRestSiblings: true`). I'd
        // recommend the dumb/simple config that I suggested
        // above instead.
        "no-unused-vars": [
                2,
                {
                        "args": "after-used",
                        "ignoreRestSiblings": false,
                        "vars": "all"
                }
        ],

        // See above.
        "notice/notice": [
                "error",
                {
                        "template": "..."
                }
        ],

        // Seems reasonable and not in eslint:recommended.
        "object-shorthand": 2,

        // A splendid rule, and not in eslint:recommended.
        "prefer-const": 2,

        // In general I like this rule, but I think it might be a bit
        // too opinionated to go in the default set.
        "prefer-template": 2,

        // A reasonable addition and not in eslint:recommended.
        "quote-props": [2, "as-needed"],

        // Not sure whether this one conflicts/duplicates relative to
        // Prettier, but the options (single, avoid-escape) seem right.
        "quotes": [2, "single", "avoid-escape"],

        // All React, so ignoring for the purposes of the default config.
        "react-hooks/rules-of-hooks": "error",
        "react/jsx-boolean-value": 2,
        "react/jsx-handler-names": 2,
        "react/jsx-key": 2,
        "react/jsx-no-bind": 2,
        "react/jsx-no-literals": 2,
        "react/jsx-sort-props": 2,
        "react/jsx-uses-react": "error",
        "react/jsx-uses-vars": 2,

        // Surprised to see this one off in Clay. I'm happy to keep
        // this out of the preset, pending agreement from others:
        // moving to TypeScript would mean less heavy reliance on JSDoc,
        // freeing it to be used to highlight actually important things
        // as opposed to redundantly encoding them.
        "require-jsdoc": 0,

        // Discussed above.
        "sort-destructure-keys/sort-destructure-keys": 2,
        "sort-imports-es6-autofix/sort-imports-es6": [
                2,
                {
                        "ignoreCase": true,
                        "ignoreMemberSort": false,
                        "memberSyntaxSortOrder": ["none", "all", "single", "multiple"]
                }
        ],

        // This rule is good, although not in eslint:recommended,
        // but I think we should accept the default (`caseSensitive: true`)
        // rather than overriding it.
        "sort-keys": [
                2,
                "asc",
                {
                        "caseSensitive": false
                }
        ],

        // Another reasonable rule, not in eslint:recommended
        "sort-vars": 2
}

Closes: #25

wincent added 3 commits March 4, 2019 09:26
Related: Sync with latest changes from liferay/clay #25
These suppressions are a bit aggressive. We're not using them elsewhere,
they override stuff that is on by default in eslint:recommended, and on
the rare occasions when you don't want the linter to complain about a
specific instance, you can add an inline suppression.

Related: #25
Based on looking at the current "develop" plugins and rules:

https://github.com/liferay/clay/blob/01432f23daa1a3abac3380b17b0545779bdefb1e/.eslintrc

Comments added inline:

```javascript
"plugins": [
        // Already included:
        "notice",

        // The intent of this one is to avoid needing
        // a Symbol.iterator polyfill for older browsers.
        // This one is probably a bit too opinionated to
        // go in the default, but we are using it in multiple
        // places and IE doesn't have Symbol.iterator...
        "no-for-of-loops",

        // Probably don't want to include this in the default
        // because not all of our projects use React.
        "react"
],
"rules": {
        // See above.
        "no-for-of-loops/no-for-of-loops": 2,

        // On by default in eslint:recommended but we override the
        // default options.
        // "ignoreRestSiblings" strikes me as unnecessarily lax.
        // I'd rather see us use options like: `{argsIgnorePattern: '^_'}`.
        "no-unused-vars": ["error", {"ignoreRestSiblings": true}],

        // See above.
        "react/jsx-uses-react": "error",
        "react/jsx-uses-vars": "error",

        // Shouldn't be needed as Prettier should handle this.
        "no-mixed-spaces-and-tabs": ["error", "smart-tabs"],

        // See above.
        "notice/notice": [
                "error",
                {
                        "templateFile": "copyright.js"
                }
        ]
}
```

And in the PR:

https://github.com/liferay/clay/pull/1548/files#diff-df39304d828831c44a2b9f38cd45289c

```javascript
"plugins": [
        // As above.
        "notice",
        "no-for-of-loops",
        "react",

        // Another React-related one that probably doesn't
        // belong in the base.
        "react-hooks",

        // This one seems fair enough.
        "sort-destructure-keys",

        // This one is a bit scary/invasive, and we haven't
        // used it outside of a PR yet, but we can try it;
        // if it works well then we should bake it in to
        // the default.
        "sort-imports-es6-autofix"
],
"rules": {
        // Should be handled by Prettier.
        "comma-dangle": [2, "always"],

        // This is a good one, and not in the default eslint:recommended.
        "default-case": 2,

        // Prettier.
        "eol-last": [2, "never"],

        // Prettier, and not on by default anyway.
        "max-len": 0,

        // Already in eslint:recommended.
        "no-console": 2,

        // See above.
        "no-for-of-loops/no-for-of-loops": 2,

        // Prettier.
        "no-mixed-spaces-and-tabs": ["error", "smart-tabs"],

        // This is a good one, and not in the default eslint:recommended.
        "no-return-assign": [2, "always"],

        // Already in eslint:recommended.
        "no-undef": 2,

        // This one is dubious, and not in default eslint:recommended.
        // (Dubious because allowing underscores after this but prohibiting
        // them in method names seems a bit arbitrary or contradictory).
        // I wouldn't put this one in the default.
        "no-underscore-dangle": [
                2,
                {
                        "allowAfterThis": true,
                        "enforceInMethodNames": true
                }
        ],

        // Note this contradicts the setting on "develop"
        // (which uses `ignoreRestSiblings: true`). I'd
        // recommend the dumb/simple config that I suggested
        // above instead.
        "no-unused-vars": [
                2,
                {
                        "args": "after-used",
                        "ignoreRestSiblings": false,
                        "vars": "all"
                }
        ],

        // See above.
        "notice/notice": [
                "error",
                {
                        "template": "/**\n * © <%= YEAR %> Liferay, Inc. <https://liferay.com>\n *\n * SPDX-License-Identifier:>
                }
        ],

        // Seems reasonable and not in eslint:recommended.
        "object-shorthand": 2,

        // A splendid rule, and not in eslint:recommended.
        "prefer-const": 2,

        // In general I like this rule, but I think it might be a bit
        // too opinionated to go in the default set.
        "prefer-template": 2,

        // A reasonable addition and not in eslint:recommended.
        "quote-props": [2, "as-needed"],

        // Not sure whether this one conflicts/duplicates relative to
        // Prettier, but the options (single, avoid-escape) seem right.
        "quotes": [2, "single", "avoid-escape"],

        // All React, so ignoring for the purposes of the default config.
        "react-hooks/rules-of-hooks": "error",
        "react/jsx-boolean-value": 2,
        "react/jsx-handler-names": 2,
        "react/jsx-key": 2,
        "react/jsx-no-bind": 2,
        "react/jsx-no-literals": 2,
        "react/jsx-sort-props": 2,
        "react/jsx-uses-react": "error",
        "react/jsx-uses-vars": 2,

        // Surprised to see this one off in Clay. I'm happy to keep
        // this out of the preset, pending agreement from others:
        // moving to TypeScript would mean less heavy reliance on JSDoc,
        // freeing it to be used to highlight actually important things
        // as opposed to redundantly encoding them.
        "require-jsdoc": 0,

        // Discussed above.
        "sort-destructure-keys/sort-destructure-keys": 2,
        "sort-imports-es6-autofix/sort-imports-es6": [
                2,
                {
                        "ignoreCase": true,
                        "ignoreMemberSort": false,
                        "memberSyntaxSortOrder": ["none", "all", "single", "multiple"]
                }
        ],

        // This rule is good, although not in eslint:recommended,
        // but I think we should accept the default (`caseSensitive: true`)
        // rather than overriding it.
        "sort-keys": [
                2,
                "asc",
                {
                        "caseSensitive": false
                }
        ],

        // Another reasonable rule, not in eslint:recommended
        "sort-vars": 2
}
```

Closes: #25
@wincent
Copy link
Contributor Author

wincent commented Mar 4, 2019

I'm going to merge this, cut a pre-release, and try it in the various projects to see whether it's going to fly or not.

@wincent wincent merged commit 5960fe4 into master Mar 4, 2019
@wincent wincent deleted the wincent/sync branch March 4, 2019 09:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync with latest changes from liferay/clay
1 participant