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

Projects
None yet
7 participants
@cjihrig
Contributor

cjihrig commented Jun 24, 2018

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
@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jun 24, 2018

<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.

@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.

@@ -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.

@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.

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.

@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

const int argc = args.Length();
CHECK_GE(argc, 3);
BufferValue path(env->isolate(), args[0]);

This comment was marked as off-topic.

@benjamingr

benjamingr Jun 24, 2018

Member

Question: Why no check on args[0]

@@ -0,0 +1,50 @@
'use strict';
const common = require('../common');

This comment has been minimized.

@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.

@cjihrig

cjihrig Jun 25, 2018

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.

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 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

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>
@cjihrig

This comment has been minimized.

Contributor

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

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>

@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.

Member

ChALkeR commented Jul 1, 2018

@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.

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

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)

@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

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

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

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

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

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

@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