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.

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

mscdex commented Jan 21, 2016

@cjihrig
Copy link
Contributor

cjihrig commented Jan 21, 2016

LGTM pending CI

@thefourtheye
Copy link
Contributor

LGTM

@mscdex
Copy link
Contributor

mscdex commented Jan 21, 2016

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
Member Author

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
Member Author

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 <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: thefourtheye <thechargingvolcano@gmail.com>
@benjamingr
Copy link
Member Author

Thanks for the review. Ready to land.

@vkurchatkin
Copy link
Contributor

-1. exporting arrow functions should be a major

@jasnell
Copy link
Member

jasnell commented Jan 22, 2016

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

@jasnell
Copy link
Member

jasnell commented Jan 22, 2016

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

@tomgco
Copy link
Contributor

tomgco commented Jan 22, 2016

-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

jasnell commented Jan 23, 2016

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

@jasnell jasnell 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
Member Author

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).

@benjamingr benjamingr closed this Jan 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem. semver-major PRs 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