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 in an "all" config #683

Merged
merged 5 commits into from
Jul 19, 2016
Merged

Add in an "all" config #683

merged 5 commits into from
Jul 19, 2016

Conversation

pfhayes
Copy link
Contributor

@pfhayes pfhayes commented Jul 15, 2016

@pfhayes pfhayes mentioned this pull request Jul 15, 2016
@ljharb
Copy link
Member

ljharb commented Jul 15, 2016

These should probably be a superset of the "recommended" config - what about these options https://github.com/yannickcr/eslint-plugin-react/blob/master/index.js#L68-L69 ?

@pfhayes
Copy link
Contributor Author

pfhayes commented Jul 15, 2016

That's a good point. I would say that "all" should be every rule in it's default configuration, and I would also say it's okay for "recommended" to deviate from default configurations (to ones that might be preferred). But I'm happy to update this to be a superset of recommended if you disagree

@pfhayes
Copy link
Contributor Author

pfhayes commented Jul 15, 2016

Also, what do you think is the best for react/jsx-sort-prop-types ? It's deprecated and replaced by react/sort-prop-types so maybe it should be excluded from all?

});
it('should export a \'all\' configuration', function() {
assert(plugin.configs.all);
Object.keys(plugin.configs.all.rules).forEach(function(configName) {
Copy link
Member

Choose a reason for hiding this comment

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

this seems like it now no longer verifies that every rule is included?

It is important to have a test that fails when someone adds a new rule but forgets to add it to "all"

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, you are right, I will fix tha

@yannickcr
Copy link
Member

Nice. Can you also update the Readme?

@yannickcr yannickcr mentioned this pull request Jul 17, 2016
9 tasks
@pfhayes
Copy link
Contributor Author

pfhayes commented Jul 18, 2016

Sure thing, I've updated the README now

@yannickcr
Copy link
Member

Thanks, can you rebase your branch? Then I think we'll be good to go :)

@pfhayes
Copy link
Contributor Author

pfhayes commented Jul 19, 2016

Done, should be good to go now

@yannickcr yannickcr merged commit 063e343 into jsx-eslint:master Jul 19, 2016
@yannickcr
Copy link
Member

Merged, thanks!

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

Successfully merging this pull request may close these issues.

3 participants