Skip to content

fs: refactor self=this to arrow #4831

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

Closed
wants to merge 5 commits into from
Closed

Conversation

benjamingr
Copy link
Member

This PR refactors self = this pattern usages to arrow functions in several places in the code.

In some places, arrow functions were already used and the self = this was simply redundant.

@benjamingr benjamingr changed the title Refactor self=this to arrow fs: refactor self=this to arrow Jan 24, 2016
@ChALkeR ChALkeR added the fs Issues and PRs related to the fs subsystem / file system. label Jan 24, 2016
@silverwind
Copy link
Contributor

@benjamingr
Copy link
Member Author

@silverwind I don't understand the CI output here. Maybe it needs to be re-run? Is it normal for a run to take 5 hours?

@silverwind
Copy link
Contributor

I think the CI is currently stuck on the ARM build bots. ARM runs usually take around an hour, not five.

@nodejs/build might need to give those RPis a nudge.

@silverwind
Copy link
Contributor

Looks like this needs some work:

# TypeError: Cannot read property 'emit' of undefined
#     at fs.js:1855:13

Don't forget to run make test 😉

@benjamingr
Copy link
Member Author

I did 0_0', I'll look into what went wrong tomorrow. Thanks.

var self = this;

const close = (fd) => {
fs.close(fd || this.fd, function(er) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably also make this an arrow for the fix :)

@tflanagan
Copy link
Contributor

It looks like we quite a few of these arrow function PR's. Some are working with each other, some against. Might be worth landing/rejecting a few of those PR's so precedence is established. I foresee a few more of these kind of PR's.

@silverwind
Copy link
Contributor

Yeah, a general policy regarding arrow functions would be nice, at least for lib. I'm not a fan of these PRs that change only a handful, would rather prefer at minimum single files to be handled per PR.

Once lib is handled, we can follow up with the prefer-arrow-callback rule to enforce the style.

cc: @nodejs/ctc

@benjamingr
Copy link
Member Author

@silverwind I'm for producing the least amount of work and keeping the code consistent. I don't mind working on a full blown PR that refactors all lib (I tend to avoid big PRs and prefer small ones because they're easier to land generally).

@tflanagan
Copy link
Contributor

@benjamingr, @silverwind I've already got one from a few months ago, #3816

It touches almost every file in lib

@silverwind
Copy link
Contributor

@tflanagan oh, I totally missed that one. Will review later.
@benjamingr I guess we can put this PR on hold until #3816 is resolved.

@benjamingr
Copy link
Member Author

Roger, I'm just looking to help. If there is already someone willing to do the dirty work by all means let them do it for me :)

@benjamingr benjamingr closed this Jan 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants