search

sitespeedio / coach

Clear Eyes. Full Hearts. Can’t Lose.
MIT License
1.21k stars 64 forks source link

Assorted lint tweaks. #122

Closed XhmikosR closed 8 years ago

XhmikosR commented 8 years ago

Please check https://github.com/sitespeedio/coach/pull/122/files?w=1 without the whitespace changes.

This should help to make the ESLint config stricter in the future.

Let me know if there any things you'd like different and I'll rebase.

tobli commented 8 years ago

All in all good changes. Are there any specific eslint rules that pass with these changes, that you think should be added to the coach config? If so I'd like to see them included in the patch as well.

XhmikosR commented 8 years ago

@tobli: rebased.

About the ESlint rules, https://github.com/giakki/uncss/blob/master/.eslintrc

XhmikosR commented 8 years ago

@soulgalore @tobli: rebased with more typo fixes.

XhmikosR commented 8 years ago

Bump

XhmikosR commented 8 years ago

OK, I'm gonna freeze the changes since the branch is getting huge.

Please review and let me know if you need me to make any changes.

XhmikosR commented 8 years ago

Bump

soulgalore commented 8 years ago

Hey @XhmikosR thanks again for the PR and the issues, I've been focusing on getting 4.0 ready. I've been thinking, I rather stay away from doing my own eslint files and want to make it as easy as possible + follow someone elses standard. I've tested eslint-config-airbnb-base in pagexray https://github.com/sitespeedio/pagexray/blob/master/package.json , I'm not 100% in love with all their linting rules but it's a standard and can help the project forward and makes it easier for people making PR. What do you think?

XhmikosR commented 8 years ago

Personally I don't like that trend of depending on others' configs. I use what I believe fits my own needs.

So in order to see what those rules are, I need to dig and find that config.

Anyway, this patch does not change the config. It simply makes things stricter so I don't think there will be any problem. You can always try this branch with your config and see. On Jun 16, 2016 17:11, "Peter Hedenskog" notifications@github.com wrote:

Hey @XhmikosR https://github.com/XhmikosR thanks again for the PR and the issues, I've been focusing on getting 4.0 ready. I've been thinking, I rather stay away from doing my own eslint files and want to make it as easy as possible + follow someone elses standard. I've tested eslint-config-airbnb-base in pagexray https://github.com/sitespeedio/pagexray/blob/master/package.json , I'm not 100% in love with all their linting rules but it's a standard and can help the project forward and makes it easier for people making PR. What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sitespeedio/coach/pull/122#issuecomment-226496850, or mute the thread https://github.com/notifications/unsubscribe/AAVVtZJlTHro0SfqE_SQxCz_eY9AXYJVks5qMVmhgaJpZM4Isx7O .

XhmikosR commented 8 years ago

So, can we get this in and revisit the ESLint rules in a separate PR?

These changes are mostly consistency ones, so as long as you don't completely change the style, they should only help making the code better.

beenanner commented 8 years ago

Changes seem good to me just need conflicts resolved @XhmikosR.

XhmikosR commented 8 years ago

@beenanner: please check one last time after the rebase. https://github.com/sitespeedio/coach/pull/122/files?w=1

Travis seems to fail, so try restarting the build.

I could have made a lot more tweaks but I lost interest after all this time without merging my patches...

beenanner commented 8 years ago

@XhmikosR understandable. Thank you again for the changes and rebase. We will be releasing 4.0 soonish, so should free up a little bit more time to review pull-request going forward if our day jobs and lives don't get in the way. ;-)