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

add missing rules from eslint-plugin-react (enforcing where necessary ) #581

Merged
merged 2 commits into from
Dec 24, 2015

Conversation

tikotzky
Copy link
Contributor

Adds all missing rules from eslint-plugin-react.

Enforces react/jsx-closing-bracket-location and react/jsx-indent-props to match react style guide.

@@ -8,12 +8,22 @@ module.exports = {
'rules': {
// Prevent missing displayName in a React component definition
'react/display-name': 0,
// Forbid certain propTypes (any, array, object)
'react/forbid-prop-types': 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this take any configuration? even if disabled, it's useful documentation to include the default options imo (same on every rule you've added)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it does take a config...
I'll add the default config to all that have a 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.

just to clarify would you want it to look like this?

'react/forbid-prop-types': [0, {forbid: ['any', 'array', 'object']}]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or would you rather me add a link pointing to the docs by each config, like this.

    // Forbid certain propTypes (any, array, object)
    // https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/forbid-prop-types.md
    'react/forbid-prop-types': 0,

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 both would be ideal - the link if people want to click through, but the default config for minimal diffs if we want to override the defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing disabled rules don't have the default config.
Is that something you would want added everywhere?

One issue I see with that is that if the default config were to change we would out of date.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally yes - that's exactly the problem. I don't really care what the defaults are (now, or later) - I only care what style we want to encourage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what about rules that are disabled? What's the reason to include the default config there?

If someone is going to override they'll probably be looking at the docs anyway to see which config they would want to select when enabling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we may want to enable the rules later without having to look up the configuration from the docs :-)

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'll start by making sure each rule that is enabled specifies a config (if it has one)

I'll update the PR late tomorrow.

@tikotzky
Copy link
Contributor Author

@ljharb I added configs for all active rules.

I also added a link to the rule docs above all the rules, I started adding by each rule but it was looking pretty messy.

I'm gonna continue adding configs to the other disabled rules.
Please let me know if you have any feedback, Thanks.

@ljharb
Copy link
Collaborator

ljharb commented Nov 15, 2015

Thanks! I'd like a few more contributors to take a look before merging it, but this LGTM.

@tikotzky
Copy link
Contributor Author

ok great, let me know if you want me to change anything or squash.

@ljharb
Copy link
Collaborator

ljharb commented Dec 23, 2015

@tikotzky could you rebase, and also add links to the rule configs in comments above the rules? Thanks!

@ljharb
Copy link
Collaborator

ljharb commented Dec 23, 2015

@tikotzky oops, i just merged another react-related PR and there's conflicts again :-/ once more?

@tikotzky tikotzky force-pushed the update-eslint-react branch from 732e253 to 6ab08ca Compare December 23, 2015 23:11
@tikotzky
Copy link
Contributor Author

ok rebased. now to add the links...

@tikotzky
Copy link
Contributor Author

I added links to all rules that were missing.

It seems like there has been some more rules added since this PR was originally created i'll go ahead and add those rules too.

@tikotzky
Copy link
Contributor Author

@ljharb should be good to go now.
Let me know if you want me to squash or make any other changes.

@ljharb
Copy link
Collaborator

ljharb commented Dec 23, 2015

Sure, let's squash this down to one or two atomic commits, and let's merge it!

@tikotzky tikotzky force-pushed the update-eslint-react branch from 4229301 to f2afce7 Compare December 24, 2015 02:39
@tikotzky
Copy link
Contributor Author

ok squashed. should be good to go 😄

ljharb added a commit that referenced this pull request Dec 24, 2015
[eslint config] [react] add missing rules from eslint-plugin-react (enforcing where necessary)
@ljharb ljharb merged commit fbf81eb into airbnb:master Dec 24, 2015
ljharb added a commit that referenced this pull request Jan 4, 2016
dustinmartin pushed a commit to dustinmartin/javascript that referenced this pull request Jan 4, 2016
gilbox pushed a commit to gilbox/javascript that referenced this pull request Mar 21, 2016
jaylaw81 pushed a commit to appirio-digital/ads-best-practices that referenced this pull request Sep 19, 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.

2 participants