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

module: Remove deprecated/unused function requireRepl. Part of #4642 #8575

Closed
wants to merge 1 commit into from
Closed

module: Remove deprecated/unused function requireRepl. Part of #4642 #8575

wants to merge 1 commit into from

Conversation

AdriVanHoudt
Copy link
Contributor

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

module

Description of change

Remove deprecated/unused function requireRepl.

@targos targos added 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. labels Sep 17, 2016
@mscdex
Copy link
Contributor

mscdex commented Sep 17, 2016

What's the usage look like in the ecosystem? /cc @ChALkeR

jasnell
jasnell approved these changes Sep 20, 2016
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 if ecosystem breakage is limited.

@ChALkeR ChALkeR self-assigned this Sep 21, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Sep 23, 2016 •
edited
Loading

requireRepl[^a-zA-Z] grep: https://gist.github.com/ChALkeR/36c79c1fa5f43d4018b783f3409b3a11

Most of those are lib/module.js copies.

LGTM.

@ChALkeR ChALkeR removed their assignment Sep 23, 2016
bnoordhuis
bnoordhuis approved these changes Sep 23, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis 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 the commit log needs to conform to the style guide.

@AdriVanHoudt
Copy link
Contributor Author

@bnoordhuis can you point me in the right direction? Not sure what is wrong with the commit log.

@bnoordhuis
Copy link
Member

See CONTRIBUTING.md in the top-level directory; for example, the first line should be <= 50 characters, links to GH issues should be Refs: or Fixes: tags, etc.

@AdriVanHoudt
Copy link
Contributor Author

Ah I see, thanks, will update soon

@AdriVanHoudt
Copy link
Contributor Author

@bnoordhuis I think it is conform now

@jasnell
Copy link
Member

jasnell commented Sep 27, 2016

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

jasnell pushed a commit that referenced this pull request Oct 7, 2016
@AdriVanHoudt @jasnell
module: Remove deprecated function requireRepl.
4d6297f 
Refs: #4642
PR-URL: #8575
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

Landed in 4d6297f. Thank you!

@jasnell jasnell closed this Oct 7, 2016
@AdriVanHoudt AdriVanHoudt deleted the remove-requireRepl branch October 8, 2016 09:16
@jasnell
Copy link
Member

jasnell commented Oct 10, 2016

@nodejs/ctc ... any objections to landing this in v7.x?

@AdriVanHoudt
Copy link
Contributor Author

No 😉

jasnell pushed a commit that referenced this pull request Oct 10, 2016
@AdriVanHoudt @jasnell
module: Remove deprecated function requireRepl.
d582193 
Refs: #4642
PR-URL: #8575
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mysticatea mysticatea mentioned this pull request Oct 26, 2016
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AdriVanHoudt @mscdex @ChALkeR @bnoordhuis @jasnell @targos