Skip to content

Conversation

@benjamingr
Copy link
Member

Another really small one, refactor a bunch of function(...){return ... } into (...) => ....

I considered also changing Client and removing the self = this but as it
is a legacy interface I don't think it's worth the trouble.

@mscdexmscdex added the http Issues or PRs related to the http subsystem. label Jan 21, 2016
@mscdex
Copy link
Contributor

@cjihrig
Copy link
Contributor

LGTM pending CI

@thefourtheye
Copy link
Contributor

LGTM

@mscdex
Copy link
Contributor

ARM CI failure is unrelated. I re-ran the linter again since there was a Jenkins failure with it the first time: https://ci.nodejs.org/job/node-test-linter/1004/

LGTM

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a defined code style on parens for single argument arrow functions?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have one. The eslint rule would be http://eslint.org/docs/rules/arrow-parens.html

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy either way, just wanted to bring it up. :)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I'd like to see us specifying these sort of things in one clear way (of course, I don't mind aligning with it).

Copy link
Member

Choose a reason for hiding this comment

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

Please can we go for always-parens around the arguments when we do these? I find the terseness without them make them harder to visually parse when reading code. I think I might have @cjihrig on my side on this at least.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I'm perfectly fine with either (both are readable IMO) but I definitely something that thing should be specified clearly in a coding styleguide.

Refactor a bunch of `function(...){return ... }` into `(...) => ...`. PR-URL: nodejs#4796 Reviewed-By: Brian White <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: thefourtheye <[email protected]>
@benjamingr
Copy link
MemberAuthor

Thanks for the review. Ready to land.

@vkurchatkin
Copy link
Contributor

-1. exporting arrow functions should be a major

@jasnell
Copy link
Member

@vkurchatkin ... I tend to agree but can you expand on the reasoning for that

@jasnell
Copy link
Member

In fact, reviewing it further, I'm going to pop the LTS watch label off this...

@tomgco
Copy link
Contributor

-1. exporting arrow functions should be a major

I agree,

The context of this can not be changed with bind or call also an exported fat arrow function will also not have a .prototype which might be relied on by implementors.

@jasnell
Copy link
Member

going to mark as semver-major defensively for now. That shouldn't stop it from landing tho if we think it's ready :-)

@jasnelljasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 23, 2016
@benjamingr
Copy link
MemberAuthor

I'm going to close this - I wanted to start discussion about the coding style in the files - look like that goal has been achieved.

As we don't have a consensus that arrows are a good idea here I'm going to close this.

(The concerns for a breaking change are unfounded though - the function doesn't use this anyway so there is no change w.r.t bind/call, the only difference is when calling it with new and I doubt anyone ever did that).

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

httpIssues or PRs related to the http subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@benjamingr@mscdex@cjihrig@thefourtheye@vkurchatkin@jasnell@tomgco@Qard@rvagg@targos