★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21498Nouvelle recherche | Portail wanayoo
Skip to content

/node

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: undeprecate lchown() #21498

Merged
merged 1 commit into from Jun 27, 2018

Conversation

Reviewers

@benjamingr benjamingr

@geek geek

Assignees
No one assigned
Labels
deprecations fs libuv
Projects
None yet
Milestone
No milestone
7 participants
@cjihrig @nodejs-github-bot @benjamingr @ChALkeR @Trott @geek @tniessen
@cjihrig
Copy link
Contributor

cjihrig commented Jun 24, 2018 •
edited

uv_fs_lchown() exists, as of libuv 1.21.0. fs.lchown() can now be undeprecated. This commit also adds tests, as there were none. As with fs.chown() and fs.fchown(), the tests are somewhat limited.

Fixes: #19868

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the libuv label Jun 24, 2018

@cjihrig

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

cjihrig commented Jun 24, 2018

CI: https://ci.nodejs.org/job/node-test-commit/19401/
@benjamingr

benjamingr reviewed Jun 24, 2018
View changes

doc/api/deprecations.md Outdated

<a id="DEP0038"></a>
### DEP0038: fs.lchownSync(path, uid, gid)

Type: Documentation-only

The [`fs.lchownSync(path, uid, gid)`][] API is deprecated.
The [`fs.lchownSync(path, uid, gid)`][] API was previously deprecated. As of

This comment has been minimized.

Sign in to view
Copy link
@benjamingr

benjamingr Jun 24, 2018

Member

Given it was never hard-deprecated or removed - I'm not sure it's not best to remove this entirely.
@benjamingr

benjamingr reviewed Jun 24, 2018
View changes

doc/api/fs.md Outdated
@@ -1918,6 +1920,8 @@ changes:
pr-url: https://github.com/nodejs/node/pull/7897
description: The `callback` parameter is no longer optional. Not passing
it will emit a deprecation warning with id DEP0013.
- version: v0.4.7

This comment has been minimized.

Sign in to view
Copy link
@benjamingr

benjamingr Jun 24, 2018

Member

I also think it makes sense to remove the deprecation entirely since there is no effect about this being deprecated in earlier versions and it was doc-deprecated anyway.
@benjamingr

benjamingr reviewed Jun 24, 2018
View changes

doc/api/fs.md Outdated
pr-url: https://github.com/nodejs/node/pull/XXXXX-replace
description: This API is no longer deprecated.
- version: v10.0.0
description: This API is deprecated.

This comment has been minimized.

Sign in to view
Copy link
@benjamingr

benjamingr Jun 24, 2018

Member

I don't see a point in keeping this deprecation regardless of the other one since fsPromises is experimental
@benjamingr

benjamingr reviewed Jun 24, 2018
View changes

src/node_file.cc
const int argc = args.Length();
CHECK_GE(argc, 3);

BufferValue path(env->isolate(), args[0]);

This comment was marked as off-topic.

Sign in to view
Copy link
@benjamingr

benjamingr Jun 24, 2018

Member

Question: Why no check on args[0]
@benjamingr

benjamingr reviewed Jun 24, 2018
View changes

test/parallel/test-fs-lchown.js
@@ -0,0 +1,50 @@
'use strict';

const common = require('../common');

This comment has been minimized.

Sign in to view
Copy link
@benjamingr

benjamingr Jun 24, 2018

Member

If there are other tests you think we should add to lchown, let me know and we'll make good-first-issues out of them

This comment has been minimized.

Sign in to view
Copy link
@cjihrig

cjihrig Jun 25, 2018

Author Contributor

We're pretty light on chown(), fchown(), and lchown() happy path tests in general. My guess is because of platform support and getting the uid and gid parts right on the CI.
@benjamingr

This comment has been minimized.

Sign in to view
Copy link
Member

benjamingr commented Jun 24, 2018

Code of commit 5a08026 LGTM but would prefer it if someone with more libuv experience approves :)

Nice work!

@cjihrig cjihrig force-pushed the cjihrig:lchown branch from 5a08026 to 85c6651 Jun 25, 2018

@cjihrig cjihrig referenced this pull request Jun 25, 2018

Closed

test: add tests for fsPromises.chown to increase coverage #20574

2 of 2 tasks complete
@geek

geek approved these changes Jun 26, 2018
View changes

@cjihrig cjihrig force-pushed the cjihrig:lchown branch 3 times, most recently from 11cd3f4 to ea9d597 Jun 27, 2018

@cjihrig
fs: undeprecate lchown() 
uv_fs_lchown() exists, as of libuv 1.21.0. fs.lchown() can now
be undeprecated. This commit also adds tests, as there were
none.

PR-URL: #21498
Fixes: #19868
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
Loading status checks…
7ff50f9

@cjihrig cjihrig force-pushed the cjihrig:lchown branch from ea9d597 to 7ff50f9 Jun 27, 2018

@cjihrig

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

cjihrig commented Jun 27, 2018

Updated as requested.

CI: https://ci.nodejs.org/job/node-test-pull-request/15652/
The inspector failures on Windows are not related.

@cjihrig cjihrig merged commit 7ff50f9 into nodejs:master Jun 27, 2018

@cjihrig cjihrig deleted the cjihrig:lchown branch Jun 27, 2018

targos added a commit that referenced this pull request Jun 28, 2018

@cjihrig @targos
fs: undeprecate lchown() 
uv_fs_lchown() exists, as of libuv 1.21.0. fs.lchown() can now
be undeprecated. This commit also adds tests, as there were
none.

PR-URL: #21498
Fixes: #19868
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
2e07d45

@shisama shisama referenced this pull request Jun 30, 2018

Merged

fs: fix fsPromises.lchmod errors on non-Mac #21435

3 of 3 tasks complete

@ChALkeR ChALkeR referenced this pull request Jul 1, 2018

Merged

doc: remove 2 unused error codes from errors.md #21491

2 of 4 tasks complete
@ChALkeR

This comment has been minimized.

Sign in to view
Copy link
Member

ChALkeR commented Jul 1, 2018 •
edited

@nodejs/tsc

According the the git history, this is the first commit which removes deprecation numbers from the documentation. Is that intended?
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Jul 1, 2018

@ChALkeR As long as the deprecation number is never re-used, I'm personally OK with removing a deprecation number from the docs if it is unused in the corresponding Node.js version. (I'm also OK with leaving it in, with or without an indication that it is no longer used.)

targos added a commit that referenced this pull request Jul 3, 2018

@targos
2018-07-04, Version 10.6.0 (Current) 
Notable changes:

* build:
  * Node.js should now be about 60% faster to startup than the previous version,
    thanks to the use V8's code cache feature for core modules. [#21405](#21405)
* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)
Loading status checks…
ff9aa5b

@targos targos referenced this pull request Jul 3, 2018

Merged

v10.6.0 proposal #21629

targos added a commit that referenced this pull request Jul 3, 2018

@targos
2018-07-04, Version 10.6.0 (Current) 
Notable changes:

* build:
  * Node.js should now be about 60% faster to startup than the previous version,
    thanks to the use V8's code cache feature for core modules. [#21405](#21405)
* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629
Loading status checks…
1f713d6

targos added a commit that referenced this pull request Jul 4, 2018

@targos
2018-07-04, Version 10.6.0 (Current) 
Notable changes:

* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
4716fd1

targos added a commit that referenced this pull request Jul 4, 2018

@targos
2018-07-04, Version 10.6.0 (Current) 
Notable changes:

* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
7951e6d

@tniessen tniessen added fs deprecations labels Sep 8, 2018

@tniessen tniessen referenced this pull request Sep 9, 2018

Closed

doc: add full deprecation history #22766

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.