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 xo as a linting tool #64

Merged
merged 1 commit into from
Oct 14, 2016
Merged

Add xo as a linting tool #64

merged 1 commit into from
Oct 14, 2016

Conversation

jfmengels
Copy link
Contributor

This adds XO as a linting tool to have a consistent linter and style accross the Zeit projects, as requested by @leo.

@leo
Copy link
Contributor

leo commented Oct 14, 2016

Looks great! But I think the esnext property is missing from the XO config 😊

return n;
return n
default:
return undefined
Copy link
Contributor Author

@jfmengels jfmengels Oct 14, 2016

Choose a reason for hiding this comment

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

I had to add a default case, which keeps the current behavior, but you might want to add a different behavior somehow (return NaN or something?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why should we add a different behavior (instead of returning undefined)?

Copy link
Contributor Author

@jfmengels jfmengels Oct 14, 2016

Choose a reason for hiding this comment

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

After reviewing the code a bit more, I don't think you should. Maybe you wanted to always return a string, but the function already returns undefined in some cases, so this should be fine.

It was more of a "Hey, you may have forgotten this case" reminder. The linter warned me of it, and I wanted to warn you too, before this warning gets lost forever ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! Let's keep this for now.

@jfmengels
Copy link
Contributor Author

Sorry, forgot to mention that.
I didn't add it on purpose, as I thought you might not want to make a breaking change. This package (probably?) works with older versions of Node, and ES2015-ifying it would necessarily be a breaking change and would limit its use to newer Node versions.

I'm fine with adding the esnext property though. Just let me know :)

@leo
Copy link
Contributor

leo commented Oct 14, 2016

@jfmengels Ahhh, you're right. We're not transpiring anything atm, so we also shouldn't add the property!

@leo leo merged commit 8e33385 into vercel:master Oct 14, 2016
@jfmengels jfmengels deleted the xo branch October 14, 2016 08:48
@jfmengels
Copy link
Contributor Author

Will probably do a few more repos later today/this week-end. Let me know if you need help for the zeit/now repo too ;)

@leo
Copy link
Contributor

leo commented Oct 14, 2016

@jfmengels Cool, thanks! 😊 Yea, I definitely need help there... What's the best way to handle that? Can you copy my branch and work from there?

@leo
Copy link
Contributor

leo commented Oct 14, 2016

Oh, we also need the badge! 😄

@jfmengels
Copy link
Contributor Author

Yea, I definitely need help there... What's the best way to handle that? Can you copy my branch and work from there?

Sure, I'll try that. And you're right, forgot the badge. Will make PRs for that ;)

@leo
Copy link
Contributor

leo commented Oct 14, 2016

@jfmengels Perfect! I already added the badge to async-throttle and ms 😊

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