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: deprecate unused function convertToContext #7829

Closed
wants to merge 5 commits into from
Closed

repl: deprecate unused function convertToContext #7829

wants to merge 5 commits into from

Conversation

princejwesley
Copy link
Contributor

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

repl

Description of change

Remove unused function convertToContext

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 21, 2016
@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 21, 2016
@thefourtheye
Copy link
Contributor

cc @ChALkeR Can you please search for this function in the npm modules?

@addaleax
Copy link
Member

Good idea, LGTM if nothing too relevant comes up (I’d be surprised if it did, this code is unused since 2010).

CI: https://ci.nodejs.org/job/node-test-commit/4216/

@Trott
Copy link
Member

Trott commented Jul 21, 2016 •
edited
Loading

LGTM. Probably worth a CITGM run too, just to flush out anything obvious early on:

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/341/

Hopefully I did that correctly....

EDIT: CITGM isn't working because of a breaking change on master. If #7846 lands, it will revert the commit that caused the regression and we can try CITGM again then.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 21, 2016

LGTM if it's not in use.

@Fishrock123
Copy link
Contributor

Is this supposed to be a user API?

@princejwesley
Copy link
Contributor Author

@Fishrock123 No.

History

@ChALkeR
Copy link
Member

ChALkeR commented Jul 24, 2016 •
edited
Loading

@thefourtheye Does not seem to be used anywhere (based on the old data from 2016-01-28, new data will be ready a bit later). As always, I checked only @latest versions.

@jasnell
Copy link
Member

jasnell commented Jul 26, 2016

Even tho this doesn't appear to be used, I would still recommend (and would be more comfortable with) a proper deprecation cycle before removing outright.

@thefourtheye
Copy link
Contributor

Considering my recent attempts with fs, I am inclined to go with @jasnell's comment.

@princejwesley
Copy link
Contributor Author

... a proper deprecation cycle before removing outright

@jasnell Using util#deprecation ?

@jasnell
Copy link
Member

jasnell commented Jul 26, 2016

We have two methods: soft deprecation (docs only) or util.deprecation. We
should likely start with soft deprecation in v7.

On Tuesday, July 26, 2016, Prince John Wesley notifications@github.com
wrote:

... a proper deprecation cycle before removing outright

@jasnell https://github.com/jasnell Using util#deprecation
https://nodejs.org/api/util.html#util_util_deprecate_function_string ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7829 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eTYlUF2uNPYwwtRiIyKQjNzB5ED2ks5qZjG8gaJpZM4JSSl3
.

@princejwesley
Copy link
Contributor Author

@jasnell FYI, convertToContext method is not currently documented in repl.md.

@addaleax
Copy link
Member

Yeah, soft deprecation isn’t really applicable here, and util.deprecate seems like the way to go.

@princejwesley
Copy link
Contributor Author

@addaleax @jasnell We need a clean up/todo label for v8.x. Do we have one?

@thefourtheye
Copy link
Contributor

@princejwesley Just a leave a to-do like this TODO(princejwesley): followed by the note.

yorkie
yorkie reviewed Jul 28, 2016
View reviewed changes
lib/repl.js
@@ -1222,7 +1222,7 @@ REPLServer.prototype.convertToContext = function(cmd) {
}

return cmd;
};
}, 'replServer.convertToContext will be removed in v8.0.0');
Copy link
Contributor

Choose a reason for hiding this comment

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

use "after", that represents a range, instead of the precise word "in" would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yorkie removed after v7 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly what I mean :-) @princejwesley

@princejwesley princejwesley changed the title repl: remove unused function convertToContext repl: deprecate unused function convertToContext Jul 28, 2016
@yorkie
Copy link
Contributor

yorkie commented Jul 28, 2016

LGTM

@addaleax
Copy link
Member

LGTM, CI: https://ci.nodejs.org/job/node-test-commit/4305/

jasnell
jasnell reviewed Jul 28, 2016
View reviewed changes
lib/repl.js
@@ -1222,7 +1224,7 @@ REPLServer.prototype.convertToContext = function(cmd) {
}

return cmd;
};
}, 'replServer.convertToContext will be removed after v7');
Copy link
Member

Choose a reason for hiding this comment

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

Our deprecation messages typically take a form like, 'replServer.convertToContext() is deprecated', without committing to a specific version when it may be removed. The message should also indicate if there is an alternative that the developer should use.

@yorkie
Copy link
Contributor

yorkie commented Jul 28, 2016 •
edited
Loading

Oops, it might need a test case, though.

@jasnell
Copy link
Member

jasnell commented Jul 28, 2016

@nodejs/ctc
LGTM

@princejwesley
Copy link
Contributor Author

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

@yorkie
Copy link
Contributor

yorkie commented Jul 30, 2016

LGTM if CI is green :-)

@ChALkeR ChALkeR mentioned this pull request Jul 31, 2016
Closed
@jasnell
Copy link
Member

jasnell commented Aug 1, 2016

@nodejs/ctc ... putting out a last call for comments on this. If there are no objections to adding this deprecation I will land tomorrow.

jasnell pushed a commit that referenced this pull request Aug 4, 2016
@princejwesley @jasnell
repl: deprecate unused function convertToContext
488d28d 
PR-URL: #7829
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

Ok, it took longer than a day but landed in 488d28d! :-)

@jasnell jasnell closed this Aug 4, 2016
@mysticatea mysticatea mentioned this pull request Oct 26, 2016
Closed
@refack
Copy link
Contributor

refack commented Jun 1, 2017

Just found this. Can we remove it?

@seishun seishun mentioned this pull request Jun 3, 2017
Merged
2 tasks
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.