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 eslint script (extends airbnb + react + react-native) #317

Merged
merged 2 commits into from
Dec 22, 2016

Conversation

cooperka
Copy link
Collaborator

@cooperka cooperka commented Dec 21, 2016

Currently 486 errors, 7 warnings!

We'll have to go through and iteratively fix all of them. Most popular IDEs have an ESLint plugin where the lints will show up as red underlines, so that makes things much easier. There's also an automatic cleanup with eslint --fix that I haven't tried yet.

Some issues should be ignored, and that can be done line-by-line with // eslint-disable-line rule-name or for blocks with /* eslint-disable rule-name */ ...code... /* eslint-enable */

The README for eslint-config-cooperka has also been updated with config steps in case you want to have a look.

As suggested in #314.

@kfiroo
Copy link
Collaborator

kfiroo commented Dec 21, 2016

@cooperka shouldn't we only have eslint-config-cooperka in our devDependencies?

@cooperka
Copy link
Collaborator Author

cooperka commented Dec 21, 2016

Yeah, good point that would be easier. I guess I was going for flexibility over usability but I agree it should just take care of that itself. I'll update soon.

Should we fix lints in this PR, or make a new one?

@kfiroo
Copy link
Collaborator

kfiroo commented Dec 21, 2016

New one i think, it might not be so friendly

@cooperka
Copy link
Collaborator Author

cooperka commented Dec 21, 2016

@kfiroo apparently npm doesn't work that way... here's my attempt.

Do you have a different suggestion for how we could use eslint-config-cooperka without also specifying the other dependencies such as eslint-config-airbnb? It seems like Airbnb has the same problem, hence their nifty script for updating automatically (which I included in my own README).

Attempting to run eslint with the above commit of course gives not found, so I also tried running node node_modules/eslint-config-cooperka/node_modules/eslint/bin/eslint.js, but it can't resolve eslint-config-cooperka/react-native relative to eslint's module.

Removed a few unnecessarily strict rules, so we're
down to 486 errors now.
@kfiroo
Copy link
Collaborator

kfiroo commented Dec 22, 2016

@cooperka Which version of npm are you using?
This sounds a lot like an npm 2 thing, but I'm not sure.

We can keep the dependencies in devDeps, just feels strange that it might fall out of sync with your code who uses it.

@cooperka
Copy link
Collaborator Author

@kfiroo I tried with npm v4.0.5 on both node v6.9.1 and v7.3.0. Have you tried yourself? I'm guessing there's no easy solution, since Airbnb couldn't even solve it.

@kfiroo
Copy link
Collaborator

kfiroo commented Dec 22, 2016

@cooperka sorry I haven't tried it yet :(
But I think you are right, I just thought you forgot to remove them this is why I asked in the first place

So, I'm merging?

@cooperka
Copy link
Collaborator Author

Sure! Assuming you're happy with the rules (but we can always change / override them later as we fix lints).

@kfiroo kfiroo merged commit 7ac0ceb into FaridSafi:master Dec 22, 2016
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