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 ESLint rules as comments #1503

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

IvanGoncharov
Copy link
Member

Idea is to first add all of them as comment and than enable on per rule
basis.

@IvanGoncharov
Copy link
Member Author

No changes to existing rules I just sorted them in the same order as official docs.
All new rules added as comments.

@mjmahone
Copy link
Contributor

mjmahone commented Sep 6, 2018

So anything that's commented out we want to have on, but we haven't turned it on yet? Any of those that can't be automatically turned on would be great to have as "simple first contribution" tasks for anyone looking to start contributing to the repo.

I think the re-order should be a separate PR: that way, it's clear in this PR which rules we want to add, and we can discuss whether those additions are valuable, before encouraging people to modify the repo to get those rules passing.

@IvanGoncharov
Copy link
Member Author

So anything that's commented out we want to have on, but we haven't turned it on yet?

No, it's just all rules that were added in ESLint. Idea is to keep existing practice of explicitly marking rules as off or error so when comparing with ESLint list we can be sure that certain rule was evaluated and explicitly rejected.
Not all ESLint rules make sense for this lib, e.g.:
https://eslint.org/docs/rules/max-lines-per-function

I extracted separate PR with rearrangements: #1511

.eslintrc.yml Outdated
# `eslint-plugin-flowtype` rule list based on `v2.50.0`
# https://github.com/gajus/eslint-plugin-flowtype#eslint-plugin-flowtype

#flowtype/array-style-complex-type: undecided
Copy link
Member Author

Choose a reason for hiding this comment

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

@mjmahone now all new rules are marked as undecided to prevent confusion

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Thank you

@IvanGoncharov
Copy link
Member Author

@mjmahone Can I merge #1511 so I could rebase this PR?

Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

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

Thanks for adding all of these!

.eslintrc.yml Outdated
# `eslint-plugin-flowtype` rule list based on `v2.50.0`
# https://github.com/gajus/eslint-plugin-flowtype#eslint-plugin-flowtype

#flowtype/array-style-complex-type: undecided
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Thank you

IvanGoncharov added a commit that referenced this pull request Sep 6, 2018
Context:
#1503 (comment)
> I think the re-order should be a separate PR: that way, it's clear in this PR which rules we want to add,
Idea is to first add all of them as comment and than enable on per rule
basis.
@IvanGoncharov IvanGoncharov merged commit d1c4e7d into graphql:master Sep 6, 2018
@IvanGoncharov IvanGoncharov deleted the eslintRules branch September 6, 2018 19:12
@leebyron
Copy link
Contributor

leebyron commented Sep 6, 2018

Note that any of these that deal with formatting (most of the flow ones) should be explicitly off if we're going to use prettier

@IvanGoncharov
Copy link
Member Author

@leebyron Thanks for the info 👍
I was planning to use this list https://github.com/prettier/eslint-config-prettier/blob/master/index.js

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

Successfully merging this pull request may close these issues.

None yet

4 participants