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

repl: remove unused function convertToContext by seishun · Pull Request #13434 · nodejs/node · GitHub

/ node Public

repl: remove unused function convertToContext - #13434

Merged
seishun merged 1 commit into
nodejs:masterfrom
seishun:remove-convertToContext
Jun 5, 2017
ConversationCommits (1)ChecksFiles changed
Merged

repl: remove unused function convertToContext#13434
seishun merged 1 commit into
nodejs:masterfrom
seishun:remove-convertToContext

Conversation

seishun commented Jun 3, 2017

Copy link
Copy Markdown
Contributor

See #7829. We forgot to remove it prior to 8.0.0 release. 😞
But better late than never.

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

repl

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jun 3, 2017

tniessen commented Jun 3, 2017

Copy link
Copy Markdown
Member

There has never been any public API documentation on that function, right?

seishun commented Jun 3, 2017

Copy link
Copy Markdown
Contributor Author

@tniessen right.

cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 3, 2017
cjihrig approved these changes Jun 3, 2017
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, but marking as semver major.

TimothyGu reviewed Jun 4, 2017
View reviewed changes

TimothyGu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

doc/api/deprecations.md should be updated to End-of-Life too, no?

jasnell previously requested changes Jun 4, 2017
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

While this may not be used or documented, it is still a publicly accessible method. It should go through a proper runtime deprecation cycle before being removed

jasnell dismissed their stale review June 4, 2017 19:56

Ha! I should double check the code before I comment ;-)

jasnell approved these changes Jun 4, 2017
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM given that it was runtime deprecated already. The documentation in deprecations.md needs to be updated tho

repl: remove unused function convertToContext
3d9e7bb 
PR-URL: #13434
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
seishun closed this Jun 5, 2017
seishun deleted the remove-convertToContext branch June 5, 2017 10:34
seishun merged commit 3d9e7bb into nodejs:master Jun 5, 2017
jasnell mentioned this pull request Oct 30, 2017
Merged
tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
tniessen mentioned this pull request Sep 7, 2018
Closed
3 tasks
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

jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
+3 more reviewers
TimothyGu TimothyGu left review comments
thefourtheye thefourtheye approved these changes
princejwesley princejwesley approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

deprecations Issues and PRs related to deprecations. repl Issues and PRs related to the REPL subsystem. 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.

9 participants

Back | FazBrowse Home | New Git URL