Skip to content

linkedlist: remove unused methods#11726

Closed
mscdex wants to merge 1 commit intonodejs:masterfrom
mscdex:linkedlist-remove-unused-methods
Closed

linkedlist: remove unused methods#11726
mscdex wants to merge 1 commit intonodejs:masterfrom
mscdex:linkedlist-remove-unused-methods

Conversation

@mscdex
Copy link
Copy Markdown
Contributor

@mscdex mscdex commented Mar 7, 2017 •
edited
Loading

This should probably only land once lib/_linklist.js is removed to avoid any breakage. Since that private module has been (runtime) deprecated since v5.0.0, should it finally be removed in v8.0.0? If not, I can mark this as 'in progress' or similar until it is removed. Thoughts @nodejs/ctc?

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • lib/src

@mscdex mscdex added lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 7, 2017
@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 7, 2017
@mscdex mscdex removed the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 7, 2017
indutny
indutny approved these changes Mar 7, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM, as long as CI is happy.

@sam-github
Copy link
Copy Markdown
Contributor

sam-github commented Mar 7, 2017

I think the entire module should be removed, as you say, its been runtime deprecated since 5.x, 8.x is a good time to remove it.

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Mar 7, 2017

@ChALkeR ... can you check to see if there is any ecosystem usage of _linked_list remaining? I'm +1 to removing it so long as it doesn't break anything (keeping in mind, of course, the fact that a stub should remain in order to keep malicious userland folks from jumping on the name)

@ChALkeR ChALkeR self-requested a review March 8, 2017 13:46
@Trott Trott mentioned this pull request Mar 13, 2017
Closed
cjihrig
cjihrig approved these changes Mar 13, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM if the ecosystem is not using it. I'd also be +1 to removing it completely if it's not being used.

@mscdex mscdex added this to the 8.0.0 milestone Mar 14, 2017
@Trott Trott mentioned this pull request Mar 20, 2017
Closed
@mscdex
Copy link
Copy Markdown
Contributor Author

mscdex commented Mar 21, 2017 •
edited
Loading

@ChALkeR Any news?

CITGM FWIW: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/666/

@rvagg
Copy link
Copy Markdown
Member

rvagg commented Mar 22, 2017

lgtm

@mscdex
Copy link
Copy Markdown
Contributor Author

mscdex commented Mar 22, 2017

To those who approved this.... is that an approval of this PR as-is or approval of just removing the module entirely? @indutny @cjihrig @jasnell @rvagg

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Mar 22, 2017 via email

@cjihrig
Copy link
Copy Markdown
Contributor

cjihrig commented Mar 22, 2017

What @jasnell said.

@Trott Trott mentioned this pull request Mar 26, 2017
Closed
@mscdex
Copy link
Copy Markdown
Contributor Author

mscdex commented Mar 29, 2017 •
edited
Loading

I've decided to do the _linklist removal separately in #12113. I can combine these two if there is interest in doing so.

@Trott Trott mentioned this pull request Apr 3, 2017
Closed
jasnell pushed a commit that referenced this pull request Apr 4, 2017
@mscdex @jasnell
linkedlist: remove unused methods
b40dab5 
PR-URL: #11726
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 4, 2017

Landed in b40dab5

@jasnell jasnell closed this Apr 4, 2017
@mscdex mscdex deleted the linkedlist-remove-unused-methods branch April 4, 2017 20:09
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@Trott Trott removed the ctc-review label Apr 6, 2017
@yosuke-furukawa yosuke-furukawa mentioned this pull request May 4, 2017
Closed
2 tasks
@ChALkeR
Copy link
Copy Markdown
Member

ChALkeR commented May 4, 2017

Sorry, it looks like I forgot this one.

It's completely ok to change, the public version of the module have been runtime-deprecated in #3078 and removed in #12113. The (old) usage data is at #3078 (comment) and was about 17 packages.

ChALkeR
ChALkeR reviewed May 4, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

Late LGTM :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChALkeR ChALkeR ChALkeR left review comments

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+2 more reviewers

@indutny indutny indutny approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@mscdex @sam-github @jasnell @rvagg @cjihrig @ChALkeR @indutny @thefourtheye @Trott @nodejs-github-bot