FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: Remove p, has been deprecated for years by geek · Pull Request #2529 · nodejs/node · GitHub

/ node Public

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

Closed
geek wants to merge 1 commit into
nodejs:masterfrom
geek:deprecate-p
ConversationCommits (1)ChecksFiles changed
Closed

util: Remove p, has been deprecated for years#2529
geek wants to merge 1 commit into
nodejs:masterfrom
geek:deprecate-p

Conversation

geek commented Aug 24, 2015

Copy link
Copy Markdown
Member

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 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 commented Aug 24, 2015

Copy link
Copy Markdown
Contributor

Adding this to the list of things to check.

brendanashworth commented Aug 24, 2015

Copy link
Copy Markdown
Contributor

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

Fishrock123 commented Aug 24, 2015

Copy link
Copy Markdown
Contributor

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

mscdex commented Aug 25, 2015

Copy link
Copy Markdown
Contributor

LGTM

geek commented Aug 31, 2015

Copy link
Copy Markdown
Member Author

@chrisdickinson update?

rvagg mentioned this pull request Sep 16, 2015
Closed

chrisdickinson commented Sep 16, 2015

Copy link
Copy Markdown
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 commented Sep 16, 2015

Copy link
Copy Markdown
Contributor

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

bnoordhuis commented Sep 16, 2015

Copy link
Copy Markdown
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 mentioned this pull request Sep 23, 2015
Closed

rvagg commented Sep 23, 2015

Copy link
Copy Markdown
Member

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

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

targos commented Oct 9, 2015

Copy link
Copy Markdown
Member

Added to the 5.0.0 milestone.
ping @Fishrock123

Fishrock123 commented Oct 10, 2015

Copy link
Copy Markdown
Contributor

LGTM

thefourtheye commented Oct 10, 2015

Copy link
Copy Markdown
Contributor

LGTM.

ChALkeR commented Oct 10, 2015

Copy link
Copy Markdown
Member

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

ChALkeR mentioned this pull request Oct 10, 2015
Closed
rvagg mentioned this pull request Oct 16, 2015
Closed
13 tasks

targos commented Oct 16, 2015

Copy link
Copy Markdown
Member

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

targos commented Oct 16, 2015

Copy link
Copy Markdown
Member

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

targos mentioned this pull request Oct 19, 2015
Merged

targos commented Oct 19, 2015

Copy link
Copy Markdown
Member

#3432

targos pushed a commit to targos/node that referenced this pull request Oct 19, 2015
util: Remove p, has been deprecated for years
8b4adb2 
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 commented Oct 19, 2015

Copy link
Copy Markdown
Member

Landed in 8b4adb2.

targos closed this Oct 19, 2015
geek added a commit that referenced this pull request Oct 21, 2015
util: Remove p, has been deprecated for years
412252c 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL