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.

Sign up for GitHub

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

Jump to bottom

linkedlist: remove unused methods #11726

Closed
wants to merge 1 commit into from
Closed

linkedlist: remove unused methods #11726

wants to merge 1 commit into from

Conversation

mscdex
Copy link
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
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
Contributor

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

rvagg commented Mar 22, 2017

lgtm

@mscdex
Copy link
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
Member

jasnell commented Mar 22, 2017 via email

@cjihrig
Copy link
Contributor

cjihrig commented Mar 22, 2017

What @jasnell said.

@Trott Trott mentioned this pull request Mar 26, 2017
Closed
@mscdex
Copy link
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
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
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
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

@cjihrig cjihrig cjihrig approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@indutny indutny indutny approved these changes

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