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

fix: npm explore spawn shell correctly #784

Closed
wants to merge 1 commit into from
Closed

fix: npm explore spawn shell correctly #784

wants to merge 1 commit into from

Conversation

jasisk
Copy link
Contributor

@jasisk jasisk commented Feb 7, 2020

What / Why

Fix npm explore <packagename> (with no additional arguments) shell spawning.

References

@jasisk jasisk requested a review from a team as a code owner February 7, 2020 22:36
@mikemimik mikemimik added Bug thing that needs fixing Community Enhancement new feature or improvement pr: needs tests requires tests before merging semver:patch semver patch level for changes labels Feb 13, 2020
@mikemimik
Copy link
Contributor

Hey @jasisk thanks for contributing this!

Is it possible for you to write a failing tests for this bug, so as your introduced fix causes the tests to pass?

Let us know if you need any help :D

@mikemimik mikemimik added the Priority Backlog a "backlogged" item that will be tracked in a Project Board label Feb 13, 2020
@jasisk
Copy link
Contributor Author

jasisk commented Feb 13, 2020 •
edited
Loading

Definitely! I wandered into the tests to see if there was already a convention for testing explore specifically but couldn't find anything that looked like the right shape. Mind pointing me to an existing test I should model it off of?

edit: happy for the free-for-all if there isn't something I should reference—I just try to leave things looking like how they were before I got there. Whatever you advise. 😀

@mikemimik
Copy link
Contributor

Hey @jasisk looks like the only place that command comes up is in test/tap/builtin-config.js. That file might be simple enough to give you some direction on how we test the cli project. Feel free to create a new file for the test you're creating for this bug, don't feel the need to clump yours in with the file I referenced.

Again, if you need any help, let us know! :D

@darcyclarke darcyclarke added this to the OSS - Sprint 11 milestone Jul 20, 2020
@isaacs
Copy link
Contributor

isaacs commented Jul 20, 2020

This LGTM for the next v6 patch release.

I'll pull into the v7 branch and add tests now, since the tests for explore are pretty lacking anyway.

@isaacs isaacs mentioned this pull request Jul 20, 2020
Closed
@jasisk
Copy link
Contributor Author

jasisk commented Jul 20, 2020

Completely forgot about this one. Thanks for pulling it in, @isaacs. Happy to take care of anything you need here.

isaacs pushed a commit that referenced this pull request Jul 20, 2020
@jasisk @isaacs
fix: npm explore spawn shell correctly
e19d1a7 
PR-URL: #784
Credit: @jasisk
Close: #784
Reviewed-by: @isaacs
isaacs pushed a commit that referenced this pull request Jul 20, 2020
@jasisk @isaacs
fix: npm explore spawn shell correctly
7d1329f 
PR-URL: #784
Credit: @jasisk
Close: #784
Reviewed-by: @isaacs
@claudiahdz claudiahdz mentioned this pull request Jul 21, 2020
Closed
@snyk-io snyk-io bot mentioned this pull request Oct 21, 2024
Open
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
Bug thing that needs fixing Enhancement new feature or improvement pr: needs tests requires tests before merging Priority Backlog a "backlogged" item that will be tracked in a Project Board semver:patch semver patch level for changes
Projects
None yet
Milestone
OSS - Sprint 11
Development

Successfully merging this pull request may close these issues.

[BUG] npm explore shell spawning without arguments immediately terminates (!isWindows)
4 participants
@jasisk @mikemimik @isaacs @darcyclarke