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

Eslint Config #20

Merged
merged 7 commits into from
Jun 4, 2019
Merged

Eslint Config #20

merged 7 commits into from
Jun 4, 2019

Conversation

schinns
Copy link
Contributor

@schinns schinns commented Jun 4, 2019

Fixes #14. In reference to conversation with @jchavarri in PR #18:

A lightweight eslint config!

In lib directory run:

$ yarn lint

P.S.- sorry for the misspelled branch name meant to name it js-lint

Copy link
Collaborator

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this @benschinn !

This is looking great, I wonder if we could add some "globals" for eslint to ignore them: https://eslint.org/docs/user-guide/configuring#specifying-globals

And eslint-disable-line statements in the existing JavaScript files, so we start from a "zero warnings" state and then move on from there? :)

lib/package-lock.json Outdated Show resolved Hide resolved
lib/.eslintrc Outdated Show resolved Hide resolved
lib/.eslintrc Outdated Show resolved Hide resolved
lib/package.json Outdated Show resolved Hide resolved
@schinns
Copy link
Contributor Author

schinns commented Jun 4, 2019

@jchavarri hey thanks for the review! I pushed up some changes that addresses most things I think.
Screen Shot 2019-06-04 at 7 57 26 AM

Now getting just one warning from lint. And it might actually be a bug.

It looks like value argument is unused:
https://github.com/jchavarri/rroo/blob/master/lib/React.js#L71

Copy link
Collaborator

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

This is awesome @benschinn ! There is just a global joo_global_objec that I think could be removed, but I can do it in a few hours. I'll merge this in at that time as well :)

I will also take a look at that unused var, definitely looks like a bug 🐛

Thanks again! 🚀

lib/.eslintrc Outdated Show resolved Hide resolved
@schinns
Copy link
Contributor Author

schinns commented Jun 4, 2019

@jchavarri should we use something like husky for linting on a pre commit hook?

@schinns
Copy link
Contributor Author

schinns commented Jun 4, 2019

Oops @jchavarri. I misunderstood your comment above and committed the esy.lock directory. should I remove it? 😬

@jchavarri
Copy link
Collaborator

should we use something like husky for linting on a pre commit hook?

@benschinn Husky is cool! Although, I would only add it if we run into linting issues pretty often, for now I'd feel inclined to keep things leaner, especially considering we'll have soon CI integration when #4 is done.

I misunderstood your comment above and committed the esy.lock directory. should I remove it?

Ah! yes please 🙏 There's no rush though, I can do it later otherwise :)

@schinns
Copy link
Contributor Author

schinns commented Jun 4, 2019

Husky is cool! Although, I would only add it if we run into linting issues pretty often, for now I'd feel inclined to keep things leaner, especially considering we'll have soon CI integration when #4 is done.

Sounds good!

Ah! yes please 🙏 There's no rush though, I can do it later otherwise :)

Done! e41bcf8

@jchavarri jchavarri merged commit b967d55 into ml-in-barcelona:master Jun 4, 2019
@jchavarri
Copy link
Collaborator

Thanks @benschinn ! ✨

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.

Add linting on js files
2 participants