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

fs: refactor promises version of lchown and lchmod #20551

Merged
merged 1 commit into from
May 15, 2018

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 6, 2018

Check for platform support first to save a level of indentation. This is just a style thing, but it simplifies the code a bit IMO.

In case anyone points it out in the diff, lchown() is currently calling fchmod() (incorrectly). I didn't update it because @ChALkeR has a fix for it in #20407.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@mscdex
Copy link
Contributor

mscdex commented May 6, 2018

What if we just did the check once during startup instead of each function call? That's what lib/fs.js currently does (except it actually doesn't assign the functions at all if symlinking isn't available).

@cjihrig
Copy link
Contributor Author

cjihrig commented May 6, 2018

Updated to check at startup.

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

I personally like the previous variant better. Because of the trace. I don't think that moving !== undefined check gives any measurable optimization and would prefer the trace to include the name of the function called.

If we want the check to be pre-calculated, I would prefer if the names of the exported functions were kept the same as they are now.

Taking into account that this changes the behavior in cases where the methods always throw, they are more likely to appear in logs, and not having the exact method name (the actual one that is not implemented) in the trace is inconvenient.

@mscdex
Copy link
Contributor

mscdex commented May 6, 2018

That's actually more of what I had in mind, just have the functions has variables and assign them to named functions in the if and else blocks.

@mscdex
Copy link
Contributor

mscdex commented May 6, 2018

It still is a little strange that we always have these functions available, which does not match the behavior of lib/fs.js, but that's for a separate PR I guess.

@cjihrig
Copy link
Contributor Author

cjihrig commented May 8, 2018

I personally like the previous variant better. Because of the trace.

I'm fine with either approach really. Just let me know which one to go with. I made the current changes in a second commit that can easily be discarded.

It still is a little strange that we always have these functions available, which does not match the behavior of lib/fs.js

I think it's wrong (or at least odd) that the functions aren't there at all. From a user's perspective, the function not existing seems more like a bug in Node, compared to receiving a nice "not implemented" error message.

@ChALkeR ChALkeR added experimental Issues and PRs related to experimental features. fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises. labels May 8, 2018
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

I'm fine with this but not sure it's critical

@cjihrig
Copy link
Contributor Author

cjihrig commented May 15, 2018

@ChALkeR I reverted to the original implementation that you signed off on.

CI: https://ci.nodejs.org/job/node-test-pull-request/14890/

Check for platform support first to save a level of indentation.

PR-URL: nodejs#20551
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Issues and PRs related to experimental features. fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants