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

doc: refactor function expressions to arrow functions #4832

Closed
wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

All the docs use arrow functions - looks like one was missed here in the fs docs.

(This is a tiny PR, I figured it's a good idea to break PRs down to a size where they're trivial to review - if bigger PRs are preferred I can do that instead)

@ChALkeR
Copy link
Member

ChALkeR commented Jan 24, 2016

Should have a doc: prefix.

@benjamingr
Copy link
Member Author

Yes, didn't tidy this up yet one bit (commit message isn't exactly great either :P) - I just wanted to know that these are welcome first.

@benjamingr benjamingr changed the title Refactor function to arrow doc: refactor function to arrow Jan 24, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Jan 24, 2016

LGTM, +1 for doc consistency.

@ChALkeR
Copy link
Member

ChALkeR commented Jan 24, 2016

One notice, though: are you sure that this was the only place?

@benjamingr
Copy link
Member Author

@ChALkeR it was the only place in that file. I have not gone through all the docs, I'm not sure what's the preferred scope of PRs (I asked about this in the issue message here). Would it be better to create multiple PRs for different files or 1 PR for going through all the docs and fixing these?

@ChALkeR
Copy link
Member

ChALkeR commented Jan 24, 2016

@benjamingr I think it's better to fix those in one commit over all the docs than a lot of smaller commits.

Previous work: #4282 (merged as 9b21119).

@benjamingr
Copy link
Member Author

Ok, thanks!

@ChALkeR
Copy link
Member

ChALkeR commented Jan 24, 2016

@benjamingr Ah, sorry. That was http-only, so I might be incorrect here.
Upd: no, it wasn't.

@ChALkeR
Copy link
Member

ChALkeR commented Jan 24, 2016

I still think that one commit that would fix all the (probably not so many) places that got missed when updating the docs would be better that a lot of smaller commits. But it would better to wait until someone else confirms that.

@ChALkeR ChALkeR added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jan 24, 2016
@benjamingr
Copy link
Member Author

I'm cool with either way, I'm just not sure what's the preferred way. Thanks for the help there is no rush whatsoever in merging this.

@ChALkeR
Copy link
Member

ChALkeR commented Jan 24, 2016

@benjamingr Ah, that PR was not http-only, it was just labeled with http label for some reason. In fact f950904 updated all of them.

@silverwind
Copy link
Contributor

There's quite a few other cases, found through ag "function ?\(.*\) ?{$" doc.

@benjamingr
Copy link
Member Author

@silverwind yes, just not in fs. I'll can gladly fix all those too I just wanted to know first if that's a good idea to do it all in one PR. If both you and @ChALkeR think it is I'll just go ahead and do it.

@silverwind
Copy link
Contributor

I'd definitely would prefer all of them in a single PR, won't matter if it's multiple commits.

@ChALkeR ChALkeR removed the fs Issues and PRs related to the fs subsystem / file system. label Jan 24, 2016
@benjamingr
Copy link
Member Author

I've updated all the docs files - put each fix in a different commit.
Thanks again for the feedback @ChALkeR and @silverwind , waiting for review.

@@ -44,7 +44,7 @@ be used as a starting-point for your own Addon.
This "Hello world" example is a simple Addon, written in C++, that is the
equivalent of the following JavaScript code:

module.exports.hello = function() { return 'world'; };
module.exports.hello = () => 'world';
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, I thought we unintended all code blocks in the docs recently, but i looks like we missed some.

Want to give it another pass, @eljefedelrodeodeljefe?

Copy link
Member Author

Choose a reason for hiding this comment

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

@silverwind if you'd like I can just fix that in the PR, I didn't want to make any opinionated changes though.

Copy link
Contributor

Choose a reason for hiding this comment

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

There ought to be more cases, let's do it in another PR, for all of doc

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, cool. I'll fix all the other spacing issues later today.

Copy link
Contributor

Choose a reason for hiding this comment

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

@silverwind must have slipped. sorry.

@benjamingr
Copy link
Member Author

Should I squash these to a single commit before merging or should it be multiple commits in a single PR?

If multiple commits - should I align the commit message of the last commit to the standard or should all commits be given special attention and detail?

Also, if multiple commits - should I add a commit for fixing issues raised in the review or should I edit the commit itself?

@silverwind
Copy link
Contributor

I'd prefer landing a single commit here. Less work for me if you squash them now, but I can do when landing.

@benjamingr benjamingr changed the title doc: refactor function to arrow doc: refactor function expressions to arrow functions Jan 24, 2016
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
This commit replaces multiple usages of `function(){}` with ES2015
arrow functions in places it was forgotten earlier. The goal is to
make the docs more consistent since other functions were already
replaced with ES2015 arrows.

In addition, it fixes invalid syntax in modules.markdown to valid
syntax as well as remove `var self = this` pattern usages in the code
where they are now possible to avoid through arrow functions.

PR-URL: #4832
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
This commit replaces multiple usages of `function(){}` with ES2015
arrow functions in places it was forgotten earlier. The goal is to
make the docs more consistent since other functions were already
replaced with ES2015 arrows.

In addition, it fixes invalid syntax in modules.markdown to valid
syntax as well as remove `var self = this` pattern usages in the code
where they are now possible to avoid through arrow functions.

PR-URL: #4832
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
This commit replaces multiple usages of `function(){}` with ES2015
arrow functions in places it was forgotten earlier. The goal is to
make the docs more consistent since other functions were already
replaced with ES2015 arrows.

In addition, it fixes invalid syntax in modules.markdown to valid
syntax as well as remove `var self = this` pattern usages in the code
where they are now possible to avoid through arrow functions.

PR-URL: nodejs#4832
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants