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 plenty of rules (#2, #3, #5, #6, #7 and #8) #11

Closed
wants to merge 6 commits into from

Conversation

jfmengels
Copy link
Contributor

@jfmengels jfmengels commented Feb 21, 2016

I implemented the rules in issues #2, #3, #5, #6, #7 and #8.

I'm not that familiar with Ava (but pretty excited about it, so I felt pretty motivated to do this), so let me know if I've got anything wrong, if there's anything to fix, or if you prefer this being split into multiple PRs.

Also, if someone can tell me why the use of const makes the tests fail in Node 0.10/0.12 but is ok on master where I also see const...

@jfmengels jfmengels changed the title I implemented the rules in issues #2, #3, #5, #6, #7 and #8. Add plenty of rules (#2, #3, #5, #6, #7 and #8) Feb 21, 2016
@sindresorhus
Copy link
Member

sindresorhus commented Feb 23, 2016

Oh wow. This is amazing!

celebration

or if you prefer this being split into multiple PRs

Yes, please. That would make it a lot easier to review and easier for you to update :)

Also, if someone can tell me why the use of const makes the tests fail in Node 0.10/0.12 but is ok on master where I also see const...

AVA transpiles the test files with Babel, but not the code itself. So you can only use const in the test files.

@jfmengels
Copy link
Contributor Author

jfmengels commented Feb 23, 2016

AVA transpiles the test files with Babel, but not the code itself. So you can only use const in the test files

Gotcha!

"or if you prefer this being split into multiple PRs"
Yes, please. That would make it a lot easier to review and easier for you to update :)

Ok. Will split it up later today then. I hope there won't be too many conflicts as some of them touch the same portions (util, index and README), but I'll handle that if it comes up.

Oh wow. This is amazing!

Glad you like it ;)

@vadimdemedes
Copy link

vadimdemedes commented Feb 23, 2016

Thanks for an amazing contribution, @jfmengels!

giphy 6

@sindresorhus
Copy link
Member

sindresorhus commented Feb 23, 2016

Lol. So much Leonardo :p

@jfmengels
Copy link
Contributor Author

jfmengels commented Feb 23, 2016

(couldn't resist)

I'll close this one, I've opened a few others (#13, #14, #15), feel free to merge however you feel, I'll come around to handle conflicts). I'll create the rest of them when those get merged, as some use code that has been added in those PRs (util.js) and merging them before will smooth things IMO.

And by the way, thanks for making this great on-hands introduction to eslint rule writing :D

@jfmengels jfmengels closed this Feb 23, 2016
@sindresorhus
Copy link
Member

sindresorhus commented Feb 23, 2016

Yes, good idea, was going to mention that. Better to handle some of them first.

200

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.

None yet

3 participants