Skip to content

Conversation

Mottoweb
Copy link
Contributor

Hey,

I stumbled upon #407, so i thought i could give it a go for a first good PR.

Let me know if i missed anything.
Things were done:

  1. Remove anything lint related from package.json
  2. Create .eslintrc.json with same rules.
  3. Remove custom rules.

@coveralls
Copy link

coveralls commented Feb 12, 2018

Coverage Status

Coverage remained the same at 91.453% when pulling 637d83b on Mottoweb:eslint-update into fec3883 on coryhouse:master.

@kwelch
Copy link
Collaborator

kwelch commented Feb 12, 2018

Thanks, I have put in a few comments. I appreciate you taking a stab at this. 👍

.eslintrc.json Outdated
@@ -0,0 +1,28 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we still want the config to live in package.json. This was done intentionally to reduce errors when the project was copied. See #405

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok no prob)

.eslintrc.json Outdated
"extends": [
"eslint:recommended",
"plugin:import/errors",
"plugin:import/warnings"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect the import plugin to be listed in plugs. Is it inherited by another?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could not trace where exactly it is inherited from but, it is.

.eslintrc.json Outdated
"plugin:import/warnings"
],
"plugins": [
"react"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that we are importing this, I do believe we should find a export preset of rules we can extend here to cover (most or all) of the rules we had previously set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then what is the point of removing custom rules and creating dependency on other packages? The very last comment section of the issue #407 suggests a PR that removes custom rules, imports default, or am i wrong?

@kwelch
Copy link
Collaborator

kwelch commented Feb 13, 2018

@coryhouse & @nickytonline What do you think? I still want to pull it down and run lint a few times and fix to see if it causes any changes.

@nickytonline
Copy link
Collaborator

I'm OK with you testing it locally to see if any other changes are required. It's using recommended/defaults for linting, so unless you see something glaring after you test it, I'd say we're probably good to go if Cory's OK with the changes.

@coryhouse
Copy link
Owner

Linting runs clean for me so PR looks 👍

@nickytonline nickytonline merged commit c01db00 into coryhouse:master Feb 13, 2018
@nickytonline
Copy link
Collaborator

Thanks for the PR @Mottoweb!

@Mottoweb
Copy link
Contributor Author

Glad to be of help!

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.

5 participants