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

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
cjihrig
cjihrig approved these changes Jun 3, 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, but marking as semver major.

TimothyGu
TimothyGu reviewed Jun 4, 2017
View reviewed changes
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
View reviewed changes
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 ;-)

jasnell
jasnell approved these changes Jun 4, 2017
View reviewed changes
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

@seishun
repl: remove unused function convertToContext
3d9e7bb 
PR-URL: #13434
Reviewed-By: Prince John Wesley <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
@tniessen tniessen mentioned this pull request Sep 7, 2018
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@princejwesley princejwesley princejwesley approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

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
@seishun @tniessen @jasnell @thefourtheye @addaleax @TimothyGu @princejwesley @cjihrig @nodejs-github-bot