doc: refactor function expressions to arrow functions#4832
Uh oh!
There was an error while loading. Please reload this page.
Conversation
ChALkeR commented Jan 24, 2016
Should have a |
benjamingr commented Jan 24, 2016
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. |
ChALkeR commented Jan 24, 2016
LGTM, +1 for doc consistency. |
ChALkeR commented Jan 24, 2016
One notice, though: are you sure that this was the only place? |
benjamingr commented Jan 24, 2016
@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 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. |
benjamingr commented Jan 24, 2016
Ok, thanks! |
ChALkeR commented Jan 24, 2016
|
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 added doc benjamingr commented Jan 24, 2016
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 commented Jan 24, 2016
@benjamingr Ah, that PR was not |
silverwind commented Jan 24, 2016
There's quite a few other cases, found through |
benjamingr commented Jan 24, 2016
@silverwind yes, just not in |
silverwind commented Jan 24, 2016
I'd definitely would prefer all of them in a single PR, won't matter if it's multiple commits. |
benjamingr commented Jan 24, 2016
I've updated all the docs files - put each fix in a different commit. |
doc/api/addons.markdown Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@silverwind if you'd like I can just fix that in the PR, I didn't want to make any opinionated changes though.
There was a problem hiding this comment.
There ought to be more cases, let's do it in another PR, for all of doc
There was a problem hiding this comment.
Ok, cool. I'll fix all the other spacing issues later today.
benjamingr commented Jan 24, 2016
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 commented Jan 24, 2016
I'd prefer landing a single commit here. Less work for me if you squash them now, but I can do when landing. |
benjamingr changed the title 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]>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]>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]>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]>
7 participants
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)