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

adding eslint #32

Merged
merged 18 commits into from
Oct 12, 2018
Merged

adding eslint #32

merged 18 commits into from
Oct 12, 2018

Conversation

dbrudner
Copy link
Contributor

I added eslint to the project. Here's a couple notes:

  • Extends airbnb rules
  • Set to use 2 space for indentation, per the pattern established in the project.
  • Set to use single quotes. There wasn't consistency across the project, I selected single quotes. It's easy to revert to double quotes by changing the setting in .eslintrc.json and running eslint --fix.
  • I changed files using jsx to have the extension .jsx instead of .js
  • I ran eslint --fix and committed the changes made by that command.
  • Instead of fixing the errors came up, I elected to ignore the rules inside .eslintrc.json. I did so because firstly, I don't know how the contributors wish to define these rules, and secondly, that potentially falls outside of the scope of the issue. There's some decisions that'll have to be made there, and I don't think I should be the person to make them.

I'd recommend moving forward that whoever's project this is make decisions about the linting rules, then remove/change the rules set in .eslintrc.json and fix the files so the code complies with the rules decided upon.

Having said that, thank you for letting me contribute. If you have any questions, comments, please let me know or comment directly in the PR and I can make changes before merging.

@dbrudner dbrudner mentioned this pull request Oct 12, 2018
@gregorysl
Copy link
Contributor

I'm curious, why

I changed files using jsx to have the extension .jsx instead of .js

@dbrudner
Copy link
Contributor Author

dbrudner commented Oct 12, 2018

@gregorysl It was caught by the linter and easy to change, so I made the change. JavaScript files containing jsx should contain that file extension. Feel free to revert if you'd like.

More on this here: airbnb/javascript#985

@dillionverma
Copy link
Owner

Thank you so much for the very thoughtful and lengthy PR.

I just looked through the lint and it looks very clean. Great work 🎉 💯

Looking more into it, I think it still might be better to keep the files named as .js as per here

Other than that, just fix the merge conflicts and I will merge this in 😄

const Card = ({
title, body, labels, issueUrl, owner, repo, number,
}) => (
=======
Copy link
Owner

Choose a reason for hiding this comment

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

Some merge conflicts remaining

@dbrudner
Copy link
Contributor Author

Resolved the conflicts, looks good to go from my end! Comment away if there's something I missed. Other than that, I completely support the decision to keep using .js files and think there's definitely a good argument for doing so. I enjoyed contributing here and would be interested in tackling more issues in the future!

@dillionverma
Copy link
Owner

Thanks so much again for contributing! I think one small remaining issue is src/index.jsx.

image

Then it is ready to merge

@dbrudner
Copy link
Contributor Author

dbrudner commented Oct 12, 2018

Encountered a second error after fixing this, both should be fixed now

@dillionverma dillionverma merged commit 8ec1239 into dillionverma:master Oct 12, 2018
@dillionverma
Copy link
Owner

Merged it in, thanks again so much for your 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.

3 participants