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

util: Remove p, has been deprecated for years #2529

Closed
wants to merge 1 commit into from

Conversation

geek
Copy link
Member

@geek geek commented Aug 24, 2015

This function has been deprecated for years... it's not documented either, so should fall under the implicit API deprecation policy:

https://github.com/joyent/node/blob/v0.8.28-release/lib/util.js#L450-L454

@brendanashworth brendanashworth added util Issues and PRs related to the built-in util module. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 24, 2015
@chrisdickinson
Copy link
Contributor

Adding this to the list of things to check.

@brendanashworth
Copy link
Contributor

+1, FWIW, here's the deprecation commit from five years ago: 022c083

@Fishrock123
Copy link
Contributor

I think it's definitely safe to remove something deprecated in v0.1.96 hahaha

@mscdex
Copy link
Contributor

mscdex commented Aug 25, 2015

LGTM

@geek
Copy link
Member Author

geek commented Aug 31, 2015

@chrisdickinson update?

@chrisdickinson
Copy link
Contributor

Still working on the tool to check. It's coming along, but I'm not sure there should be a rush to remove this?

@chrisdickinson
Copy link
Contributor

(OTOH, it's really unlikely that removing .p will break anything — I just lean towards the side of paranoia.)

@bnoordhuis
Copy link
Member

For the record, util.p was deprecated almost five years ago in commit e38eb0c. It's been printing a deprecation warning since node v0.3.0.

@rvagg
Copy link
Member

rvagg commented Sep 23, 2015

TSC agreed to remove util.p in master, I believe this can be merged but @Fishrock123 has the action item for this

@rvagg rvagg removed the tsc-agenda label Sep 23, 2015
@targos targos added this to the 5.0.0 milestone Oct 9, 2015
@targos
Copy link
Member

targos commented Oct 9, 2015

Added to the 5.0.0 milestone.
ping @Fishrock123

@Fishrock123
Copy link
Contributor

LGTM

@thefourtheye
Copy link
Contributor

LGTM.

@ChALkeR
Copy link
Member

ChALkeR commented Oct 10, 2015

Quick grep results for (sys|util)\.p\(:

deck-node-1.0.11.tgz/typed/async/async-tests.ts:185:    function () { sys.p('one'); },
deck-node-1.0.11.tgz/typed/async/async-tests.ts:186:    function () { sys.p('two'); },
deck-node-1.0.11.tgz/typed/async/async-tests.ts:187:    function () { sys.p('three'); }
definitively-typed-0.0.1.tgz/async/async-tests.ts:243:    function () { sys.p('one'); },
definitively-typed-0.0.1.tgz/async/async-tests.ts:244:    function () { sys.p('two'); },
definitively-typed-0.0.1.tgz/async/async-tests.ts:245:    function () { sys.p('three'); }
mysql-native-prerelease-1.4.2.tgz/examples/myhttp.js:8:process.addListener('uncaughtException', function(err) { sys.p(err); });
mysql-native-prerelease-1.4.2.tgz/examples/myhttp.js:40:  sys.p(q);
mysql-native-prerelease-1.4.2.tgz/tests/test_execute.js:11:examplecmd.on('error', function(s) { sys.p(s); } );
mysql-native-prerelease-1.4.2.tgz/tests/test_stress.js:9:sys.p(numclients);
noblerecord-v1.0.1.tgz/src/mysql.js:154:                sys.p(me.connection.connectError);
restler-aaronblohowiak-0.0.2.tgz/test/multipartform.js:37:             sys.p(bytesWritten);
restler-aaronblohowiak-0.0.2.tgz/test/multipartform.js:72:     sys.p("closing and sending");
shoutcast-0.0.2.tgz/lib/file.js:23:                sys.p(erro);
webidl.js-0.1.0.tgz/scratch/test.js:22:    sys.p(e);

LGTM

@targos
Copy link
Member

targos commented Oct 16, 2015

CI before landing: https://ci.nodejs.org/job/node-test-commit/861/

@targos
Copy link
Member

targos commented Oct 16, 2015

@geek this breaks a test. Do you have time to fix it ?

@targos
Copy link
Member

targos commented Oct 19, 2015

#3432

targos pushed a commit to targos/node that referenced this pull request Oct 19, 2015
Update deprecation test to use another method.

Ref: nodejs#2529
PR-URL: nodejs#3432
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@targos
Copy link
Member

targos commented Oct 19, 2015

Landed in 8b4adb2.

@targos targos closed this Oct 19, 2015
geek added a commit that referenced this pull request Oct 21, 2015
Update deprecation test to use another method.

Ref: #2529
PR-URL: #3432
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants