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

repl: remove unused function convertToContext #13434

Merged
merged 1 commit into from
Jun 5, 2017
Merged

repl: remove unused function convertToContext #13434

merged 1 commit into from
Jun 5, 2017

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Jun 3, 2017

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 nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jun 3, 2017
@tniessen
Copy link
Member

tniessen commented Jun 3, 2017

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

@seishun
Copy link
Contributor Author

seishun commented Jun 3, 2017

@tniessen right.

@cjihrig cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 3, 2017
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, but marking as semver major.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

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

jasnell
jasnell previously requested changes Jun 4, 2017
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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 jasnell dismissed their stale review June 4, 2017 19:56

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

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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

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 seishun closed this Jun 5, 2017
@seishun seishun deleted the remove-convertToContext branch June 5, 2017 10:34
@seishun seishun merged commit 3d9e7bb into nodejs:master Jun 5, 2017
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
@tniessen tniessen mentioned this pull request Sep 7, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

9 participants