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

module: Remove unused code #4649

Closed
wants to merge 1 commit into from

Conversation

zeusdeux
Copy link
Contributor

Remove unused requireRepl code.
It was deprecated after 5.1.x.

This is with reference to #4642.

Remove unused `requireRepl` code.
It was deprecated after 5.1.x
@r-52 r-52 added the module Issues and PRs related to the module subsystem. label Jan 12, 2016
@benjamingr
Copy link
Member

Fwiw - This is a breaking change. I don't really see the rush in removing it after it got a deprecation notice.

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 12, 2016
@mscdex
Copy link
Contributor

mscdex commented Jan 12, 2016

I agree, the deprecation notice was only added in November in ee72ee7. I think we would remove this in 7.0?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 12, 2016

Maybe we should add the future label and come back to this once the 6.x branch has been cut.

@zeusdeux
Copy link
Contributor Author

@mscdex The deprecation notice was added after 5.1.x and before 5.2.x and since master right now is 6.0.0-pre, I assumed I could clean it up as per the deprecation rules node follows. Also, since it was mentioned in #4642, I went ahead and removed it in preparation for 6.0.

@mscdex
Copy link
Contributor

mscdex commented Jan 12, 2016

@zeusdeux I could be wrong, but I was under the assumption that the current "deprecation policy" was to have a deprecation for the duration of (at least) one full major version before removal.

That said, this could still be pushed to master, but just not pulled in/released until 7.0.

@jasnell
Copy link
Member

jasnell commented Jan 12, 2016

@chrisdickinson ... can we possibly get some information on if the removed method is used at all in modules?

@benjamingr
Copy link
Member

I checked before my first comment through GH code search. It appears in multiple npm and GH repos. Not a very high amount but it does appear in some ones with ~10 stars.

Mostly, I don't really see the need to remove it - the benefit in removing deprecated methods is that it simplifies the code base and makes things easier for developers - not really the case here. To compare, no one is even considering removing fs. exists (and rightly so)

@jasnell
Copy link
Member

jasnell commented Jan 13, 2016

If there are modules using it, I'd say there's no harm in keeping it for at least a while longer.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

given the comments on this one, I'm inclined to close. We can revisit it later.

@jasnell jasnell closed this Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module 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.

8 participants