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

Cleaned up commit history for pr#1548 #1648

Merged
merged 7 commits into from
Mar 15, 2019

Conversation

bryceosterhaus
Copy link
Member

No description provided.

@jbalsas
Copy link
Contributor

jbalsas commented Mar 11, 2019

If everyone's happy with this, I'm happy with it! 👍

Copy link
Member

@matuzalemsteles matuzalemsteles left a comment

Choose a reason for hiding this comment

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

LGTM!

"jsx": true
}
},
"extends": "liferay",
Copy link
Contributor

Choose a reason for hiding this comment

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

If you update to the latest version fo the config (v3.0.0) you can drop some of the rules because I rolled them in to the defaults (current "master" defaults here).

For example, I think you could drop the no-for-of-loops and notice plug-ins (eslint-config-liferay includes it and will use the copyright.js file automatically if it exists).

I think you can also drop some rules:

  • max-len, eol-last, comma-dangle, no-mixed-spaces-and-tabs, and (I think) quotes: all handled by the prettier preset, which is bundled with the liferay preset.
  • no-console, no-undef: already in eslint:recommended, which the preset extends.
  • default-case, no-return-assign, object-shorthand, prefer-const, quote-props: already in the preset, because I added them there.
  • no-unused-vars: already in the preset, albeit with some different options; I'd suggest trying to see if dropping it is viable.

Basically, I think most of the plug-ins and rules can go away, except the React-related ones, and the sorting ones (which ended up being too invasive to bake into the preset).

If you're curious about which rules I added to the preset, I wrote it all up in liferay/eslint-config-liferay#25 — one of the things I did was look at this PR and the existing config on the "develop" branch to see what could be merged "upstream" into the preset.

@bryceosterhaus
Copy link
Member Author

@wincent updated. I left in the no-unused-vars rule due to ignoreRestSiblings being used fairly often with react by doing ...otherProps

@wincent
Copy link
Contributor

wincent commented Mar 13, 2019

I left in the no-unused-vars rule due to ignoreRestSiblings being used fairly often with react by doing ...otherProps

Good call. And I think we might want to make a React-variant of the config for precisely this kind of thing plus the other React stuff. I think in theory it is possible to create a react.js file in eslint-config-liferay and then you'd be able to extend "eslint-config-liferay/react" instead of "eslint-config-liferay"; I'll create an issue for setting that up: liferay/eslint-config-liferay#36

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

Successfully merging this pull request may close these issues.

5 participants