lib: deprecate fd usage for fs.truncate and fs.truncateSync by r1cebank · Pull Request #15990 · nodejs/node

r1cebank · 2017-10-06T18:16:30Z

Address issue #15753

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

Affected core subsystem(s)

BridgeAR

We need a note in the deprecations.md about the new deprecation.

BridgeAR · 2017-10-06T23:43:33Z

lib/fs.js

Nit - this should be indented with two more spaces while landing. The same applies to the other deprecation messages.

The message itself could also be improved by not saying In the future ... but just Please use ... instead.

I agree with switching to Please use .... Additionally, the occurrences of "with file descriptor" should likely be "with a file descriptor"

r1cebank · 2017-10-07T00:40:37Z

@cjihrig @BridgeAR I made some updates to the message and deprecations.md. Let me know if there is anything else I need to fix.

seishun · 2017-10-07T09:45:44Z

doc/api/deprecations.md

"if you need" is redundant IMO. Also there should be a period at the end of the sentence.

cjihrig · 2017-10-07T19:40:58Z

doc/api/deprecations.md

I think we're supposed to be using DEP00XX when authoring these deprecations. Then, when the PR is landed, the committer can replace it with a number.

seishun · 2017-10-07T19:51:04Z

lib/fs.js

Please use util.deprecate() for runtime deprecations. See how it's done here.

I asked one of the mentor during the event, and looks like for this case since we are deprecating a particular usage, we cannot use util.deprecate().

Indeed. But we still need to make sure this warning is only emitted once and that it respects --no-deprecation. See how it's done with Buffer here (although that's a pending deprecation for now, so make sure to use process.noDeprecation instead of pendingDeprecation)

Oh I see what you mean, thank you for sharing the resources, I will make changes according to what you said.

jasnell · 2017-10-10T00:30:03Z

doc/api/fs.md

Minor nit: this line is > 80 chars and will need to be wrapped before landing.

jasnell · 2017-10-10T00:31:37Z

lib/fs.js

This part is unnecessary. The emitWarning function will handle the noDeprecation configuration property internally.

This was suggested by one of the reviewers, I am just here to learn how nodejs works internally. If you think emitWarning is sufficient, i can make the change.

jasnell · 2017-10-10T00:32:03Z

Ping @nodejs/tsc

r1cebank · 2017-10-10T00:37:40Z

Can someone give me a clear idea how this should be fixed? I feel all this back and forth commenting just made me more confused.

jasnell · 2017-10-10T00:54:23Z

@r1cebank ... first of all, thank you very much for the contribution, it really is quite appreciated. My apologies if the reviews were not clear. Essentially there are a couple of things here that should be reworked slightly.

Namely, I would change your code here:

const doFlaggedDeprecation = noDeprecation ?
   function() {} : showFlaggedDeprecation;
 
 function showFlaggedDeprecation() {
   process.emitWarning(
     'Using fs.truncate with a file descriptor is deprecated. Please use ' +
     'fs.ftruncate with a file descriptor instead.',
     'DeprecationWarning', 'DEP00XX');
 }

To the following:

let truncateWarn = true;
function doFlaggedDeprecation() {
  if (!truncateWarn) {
    process.emitWarning(
      'Using fs.truncate with a file descriptor is deprecated. Please use ' +
      'fs.ftruncate with a file descriptor instead.',
      'DeprecationWarning', 'DEP00XX');
    truncateWarn = false;
  }
}

There are two reasons:

Checking the noDeprecation flag in process.binding('config') is done automatically by emitWarning when DeprecationWarning is used. There is no need for you to do that here.
Adding the truncateWarn check ensures that the deprecation warning is only emitted once.

r1cebank · 2017-10-10T00:59:55Z

Thank you @jasnell for your wonderful explanation. I think I was little confused with the emit error only once part. Thanks for the code sample, this really helps with my understanding of Node.js. I will make these changes as soon as possible.

r1cebank · 2017-10-10T04:43:41Z

I made the changes @jasnell I hope this addressed all of your concerns.

addaleax · 2017-10-10T04:46:51Z

lib/fs.js

Just a suggestion, but I’d include truncate in the function name here because it’s specific to fs.truncate(Sync)

Very good suggestion, I will keep this in mind in my future works. Thanks

jasnell

Thank you!

trevnorris

missed a space in the doc change and the git message in the second commit is too long (max 50 chars). Thanks for the patch.

trevnorris · 2017-10-11T12:29:18Z

doc/api/deprecations.md

missed a space "descriptor has"

r1cebank · 2017-10-11T17:17:40Z

I have fixed both commit messages + missing space, thanks for the suggestion.

joyeecheung · 2017-10-12T03:56:07Z

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

r1cebank · 2017-10-12T17:14:04Z

Anyone know if I can get read permission for jenkins? I cannot check the status of the build atm.

jasnell · 2017-10-13T00:48:17Z

Failure in CI is unrelated.

PR-URL: #15990 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell · 2017-10-13T01:07:15Z

Landed in c885ea7

PR-URL: nodejs/node#15990 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 6, 2017

r1cebank changed the title ~~lib: deprecate fd usage for fs.truncate and fs.truncateSync #15753~~ lib: deprecate fd usage for fs.truncate and fs.truncateSync Oct 6, 2017

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017

BridgeAR reviewed Oct 6, 2017

View reviewed changes

seishun reviewed Oct 7, 2017

View reviewed changes

doc/api/deprecations.md Outdated

Copy link

Contributor

seishun Oct 7, 2017

Choose a reason for hiding this comment

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

"if you need" is redundant IMO. Also there should be a period at the end of the sentence.

cjihrig reviewed Oct 7, 2017

View reviewed changes

seishun added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 7, 2017

seishun reviewed Oct 7, 2017

View reviewed changes

jasnell reviewed Oct 10, 2017

View reviewed changes

doc/api/fs.md Outdated

Copy link

Member

jasnell Oct 10, 2017

Choose a reason for hiding this comment

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

Minor nit: this line is > 80 chars and will need to be wrapped before landing.

jasnell reviewed Oct 10, 2017

View reviewed changes

jasnell requested a review from a team October 10, 2017 00:32

addaleax approved these changes Oct 10, 2017

View reviewed changes

jasnell approved these changes Oct 10, 2017

View reviewed changes

trevnorris approved these changes Oct 11, 2017

View reviewed changes

doc/api/deprecations.md Outdated

Copy link

Contributor

trevnorris Oct 11, 2017

Choose a reason for hiding this comment

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

missed a space "descriptor has"

r1cebank added 2 commits October 11, 2017 10:13

lib: deprecate fd usage for fs.truncate(Sync)

927cf8d

doc: deprecation note for fs.truncate(Sync)

2ff66b9

cjihrig approved these changes Oct 13, 2017

View reviewed changes

jasnell closed this Oct 13, 2017

jasnell added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 13, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

benjamingr mentioned this pull request Nov 15, 2017

fs.truncate accepts an fd but it's poorly documented #15753

Closed

tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018

Uh oh!

Conversation

r1cebank commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r1cebank commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 10, 2017

Uh oh!

r1cebank commented Oct 10, 2017

Uh oh!

jasnell commented Oct 10, 2017

Uh oh!

r1cebank commented Oct 10, 2017

Uh oh!

r1cebank commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

trevnorris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r1cebank commented Oct 11, 2017

Uh oh!

joyeecheung commented Oct 12, 2017

Uh oh!

r1cebank commented Oct 12, 2017

Uh oh!

jasnell commented Oct 13, 2017

Uh oh!

jasnell commented Oct 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

r1cebank commented Oct 6, 2017 •

edited

Loading

r1cebank commented Oct 7, 2017 •

edited

Loading