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: allow autocompletion for scoped packages #10296

Closed
wants to merge 1 commit into from
Closed

repl: allow autocompletion for scoped packages #10296

wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

Description of change

Previously, autocompletion of scoped packages was not supported by the
repl due to not including the @ character in the regular expression.

@nodejs-github-bot nodejs-github-bot added repl Issues and PRs related to the REPL subsystem. lts-watch-v6.x labels Dec 15, 2016
@evanlucas
Copy link
Contributor Author

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

bnoordhuis
bnoordhuis reviewed Dec 16, 2016
View reviewed changes
test/parallel/test-repl-tab-complete.js
});
if (!files.length) common.rimrafSync(node_modules);
}));
}
Copy link
Member

Choose a reason for hiding this comment

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

Creating files or directories outside of common.tmpDir is kind of yuck. I think it should be possible to process.chdir(common.fixturesDir), then you can check in @nodejsscope/ into the existing node_modules directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% agree. I'll try and get that working. That should simplify a lot of this test. Thanks!

@evanlucas
Copy link
Contributor Author

Updated. PTAL. Thanks! CI: https://ci.nodejs.org/job/node-test-pull-request/5437/

bnoordhuis
bnoordhuis approved these changes Dec 16, 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 with a comment.

test/common.js
@@ -60,6 +60,7 @@ function rimrafSync(p) {
rmdirSync(p, e);
}
}
exports.rimrafSync = rimrafSync;
Copy link
Member

Choose a reason for hiding this comment

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

You can drop this change now.

@evanlucas
repl: allow autocompletion for scoped packages
d04b6ff 
Previously, autocompletion of scoped packages was not supported by the
repl due to not including the `@` character in the regular expression.
jasnell pushed a commit that referenced this pull request Dec 23, 2016
@evanlucas @jasnell
repl: allow autocompletion for scoped packages
e248f7f 
Previously, autocompletion of scoped packages was not supported by the
repl due to not including the `@` character in the regular expression.

PR-URL: #10296
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Dec 23, 2016

Landed in e248f7f

@jasnell jasnell closed this Dec 23, 2016
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas added a commit that referenced this pull request Jan 3, 2017
@evanlucas
repl: allow autocompletion for scoped packages
a402652 
Previously, autocompletion of scoped packages was not supported by the
repl due to not including the `@` character in the regular expression.

PR-URL: #10296
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request Jan 4, 2017
@evanlucas
repl: allow autocompletion for scoped packages
6796bf4 
Previously, autocompletion of scoped packages was not supported by the
repl due to not including the `@` character in the regular expression.

PR-URL: #10296
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas deleted the replscope branch January 5, 2017 23:23
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@evanlucas @MylesBorins
repl: allow autocompletion for scoped packages
b38be64 
Previously, autocompletion of scoped packages was not supported by the
repl due to not including the `@` character in the regular expression.

PR-URL: #10296
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

this will need to be manually backported to v4.x

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@evanlucas @MylesBorins
repl: allow autocompletion for scoped packages
6d92391 
Previously, autocompletion of scoped packages was not supported by the
repl due to not including the `@` character in the regular expression.

PR-URL: #10296
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@evanlucas @MylesBorins
repl: allow autocompletion for scoped packages
57c4c6f 
Previously, autocompletion of scoped packages was not supported by the
repl due to not including the `@` character in the regular expression.

PR-URL: #10296
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Feb 21, 2017
@MylesBorins
2017-02-21, Version 6.10.0 'Boron' (LTS)
1b6fd44 
Notable Changes:

The SEMVER-MINOR changes include:

*  crypto: allow adding extra certs to well-known CAs (Sam Roberts)
   #9139
*  deps: Upgrade INTL ICU to version 58 (Steven R. Loomis)
   #9234
*  process: add `process.memoryUsage.external` (Fedor Indutny)
   #9587
*  src: add wrapper for process.emitWarning() (Sam Roberts)
   #9139

Notable SEMVER-PATCH changes include:

*  fs: cache non-symlinks in realpathSync. (Jeremy Yallop)
   #10253
*  repl: allow autocompletion for scoped packages (Evan Lucas)
   #10296
MylesBorins added a commit that referenced this pull request Feb 22, 2017
@MylesBorins
2017-02-21, Version 6.10.0 'Boron' (LTS)
aecb085 
Notable Changes:

The SEMVER-MINOR changes include:

*  crypto: allow adding extra certs to well-known CAs (Sam Roberts)
   #9139
*  deps: Upgrade INTL ICU to version 58 (Steven R. Loomis)
   #9234
*  process: add `process.memoryUsage.external` (Fedor Indutny)
   #9587
*  src: add wrapper for process.emitWarning() (Sam Roberts)
   #9139

Notable SEMVER-PATCH changes include:

*  fs: cache non-symlinks in realpathSync. (Jeremy Yallop)
   #10253
*  repl: allow autocompletion for scoped packages (Evan Lucas)
   #10296

PR-URL: #10974
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
@imyller
2017-02-21, Version 6.10.0 'Boron' (LTS)
edbc1bf 
    Notable Changes:

    The SEMVER-MINOR changes include:

    *  crypto: allow adding extra certs to well-known CAs (Sam Roberts)
       nodejs/node#9139
    *  deps: Upgrade INTL ICU to version 58 (Steven R. Loomis)
       nodejs/node#9234
    *  process: add `process.memoryUsage.external` (Fedor Indutny)
       nodejs/node#9587
    *  src: add wrapper for process.emitWarning() (Sam Roberts)
       nodejs/node#9139

    Notable SEMVER-PATCH changes include:

    *  fs: cache non-symlinks in realpathSync. (Jeremy Yallop)
       nodejs/node#10253
    *  repl: allow autocompletion for scoped packages (Evan Lucas)
       nodejs/node#10296

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
@imyller
2017-02-21, Version 6.10.0 'Boron' (LTS)
f99e583 
    Notable Changes:

    The SEMVER-MINOR changes include:

    *  crypto: allow adding extra certs to well-known CAs (Sam Roberts)
       nodejs/node#9139
    *  deps: Upgrade INTL ICU to version 58 (Steven R. Loomis)
       nodejs/node#9234
    *  process: add `process.memoryUsage.external` (Fedor Indutny)
       nodejs/node#9587
    *  src: add wrapper for process.emitWarning() (Sam Roberts)
       nodejs/node#9139

    Notable SEMVER-PATCH changes include:

    *  fs: cache non-symlinks in realpathSync. (Jeremy Yallop)
       nodejs/node#10253
    *  repl: allow autocompletion for scoped packages (Evan Lucas)
       nodejs/node#10296

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@princejwesley princejwesley princejwesley approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@evanlucas @jasnell @MylesBorins @bnoordhuis @addaleax @Fishrock123 @princejwesley @cjihrig @nodejs-github-bot