Added `-q` (quiet) option to `push`, `popd`, `dirs` functions. #777

alexreg · 2017-10-04T20:45:04Z

Resolves issue #753.

No unit tests added yet; I'm not familiar with the framework, and not sure how best to do them.

nfischer

Thanks for the PR!

Get rid of yarn.lock.

Also, we'll need unit tests before merging. Tests are a little tricky, try something like:

test('foobar', t => {
  try {
    common.config.silent = false;
    mocks.init(); // see test/echo.js for examples
    // do a command with -q
    const stdout = mocks.stdout(); // same with stderr, compare both values, etc.
  } finally {
    mocks.restore();
  }
}

Make sure to also move shell.config.silent = true into test.beforeEach if you follow this approach.

nfischer · 2017-10-06T01:13:05Z

src/dirs.js

@@ -173,6 +178,7 @@ function _dirs(options, index) {

  options = common.parseOptions(options, {
    'c': 'clear',
+    'q': 'quiet',


This doesn't seem right. In my zsh, dirs does not support the -q option:

❯ dirs -q zsh: bad option: -q

I only see -c, -l, -p, -v for this command.

What's the alternative? I thought we already agreed on having a -q option, even though it isn't present on any shells (that I know of).

zsh has the -q flag for pushd and popd. dirs does not have this flag to my knowledge.

nfischer · 2017-10-06T04:02:16Z

src/dirs.js

@@ -95,7 +97,7 @@ function _pushd(options, dir) {
  }

  _dirStack = dirs;
-  return _dirs('');
+  return _dirs(options.quiet ? 'q' : '');


I think you want '-q', not 'q'.

alexreg · 2017-10-06T16:44:40Z

@nfischer Okay, have a look now, I think things should be in order. :-)

nfischer

Mostly good.

I don't have a strong opposition to dirs -q. It makes logical sense compared to pushd/popd. If dirs ever does add a -q flag which differs in behavior, then we'll have to change. @freitagbr thoughts?

nfischer · 2017-10-08T02:20:35Z

test/popd.js

@@ -114,3 +115,22 @@ test('Test that rootDir is not stored', t => {
  shell.popd(); // no more in the stack
  t.truthy(shell.error());
 });
+
+test('quiet mode', t => {


Could we also test stdout for the case where -q is not passed? The test should have the same layout.

Almost did this before, but wasn't sure if you'd want them. Done now.

nfischer · 2017-10-08T02:25:16Z

Oh, this needs docs generated. Please run npm run gendocs

alexreg · 2017-10-08T02:28:22Z

Mostly good.

I don't have a strong opposition to dirs -q. It makes logical sense compared to pushd/popd. If dirs ever does add a -q flag which differs in behavior, then we'll have to change. @freitagbr thoughts?

Yep, those were my thoughts too.

alexreg · 2017-10-08T03:15:30Z

That's all done now. Word of warning: the existing code for pushd and popd writes to stderr, which differs in behaviour from the shell builtins for bash & zsh, from what I can tell. I've left it writing to stderr, but you may want to change that. (I don't know what function you'd want to call other that common.log, if so...)

nfischer

LGTM % comment. Thanks!

nfischer · 2017-10-09T03:17:36Z

test/popd.js

  try {
    shell.config.silent = false;
+    shell.pushd('test/resources/pushd');


I think this should be moved up one line, otherwise it will print during the test (which we try to avoid). Same thing for the quite mode on test.

nfischer · 2017-10-09T03:20:50Z

the existing code for pushd and popd writes to stderr, which differs in behaviour from the shell builtins

Yup, you're right. This is unfortunate legacy behavior. Feel free to file a bug and we'll get to it for a breaking release.

alexreg · 2017-10-09T11:25:38Z

It prints before the mock is initialised though. Isn’t that aufficient?

…

Sent from my iPhone

On 9 Oct 2017, at 04:19, Nate Fischer ***@***.***> wrote: @nfischer requested changes on this pull request. LGTM % comment. Thanks! In test/popd.js: > try { shell.config.silent = false; + shell.pushd('test/resources/pushd'); I think this should be moved up one line, otherwise it will print during the test (which we try to avoid). Same thing for the quite mode on test. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

nfischer · 2017-10-09T21:10:05Z

It prints before the mock is initialised though. Isn’t that aufficient?

You're correct: it doesn't affect correctness, but test results would be difficult to read if we let integration tests print to the console. That's why we use shell.config.silent in the first place during our tests.

…of silent flag.

alexreg · 2017-10-10T02:06:08Z

No problem. That's done now.

codecov-io · 2017-10-10T02:20:31Z

Codecov Report

Merging #777 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #777      +/-   ##
==========================================
+ Coverage   95.39%   95.39%   +<.01%     
==========================================
  Files          33       33              
  Lines        1237     1239       +2     
==========================================
+ Hits         1180     1182       +2     
  Misses         57       57

Impacted Files	Coverage Δ
src/dirs.js	`97.18% <100%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c889075...2168ee5. Read the comment docs.

nfischer

No problem. That's done now.

Thanks!

Added -q (quiet) option to push, popd, dirs functions.

64e8dc2

nfischer requested changes Oct 6, 2017

View reviewed changes

Added unit tests for pushd/popd quiet mode.

d55b1a1

nfischer requested changes Oct 8, 2017

View reviewed changes

nfischer requested a review from freitagbr October 8, 2017 02:23

alexreg added 2 commits October 8, 2017 04:13

Added tests for pushd and popd with quiet mode off.

fe7bf34

Updated docs for pushd and popd functions.

183d1df

nfischer requested changes Oct 9, 2017

View reviewed changes

Moved preliminary pushd commands for popd tests before disabling …

2168ee5

…of silent flag.

nfischer approved these changes Oct 10, 2017

View reviewed changes

nfischer merged commit cea0e58 into shelljs:master Oct 10, 2017

nfischer mentioned this pull request Oct 10, 2017

Feature request: pushd/popd -q option #753

Closed

snyk-bot mentioned this pull request Aug 13, 2019

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 ASLANtechnology/serverless-api-cicd#2

Open

This was referenced Aug 29, 2019

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 iliutastoica/strapi-examples#2

Open

[Snyk] Upgrade shelljs from 0.7.0 to 0.8.3 MailOnline/mol-cz-lerna-changelog#1

Open

snyk-bot mentioned this pull request Sep 19, 2019

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 ali8889/byteball#2

Open

snyk-bot mentioned this pull request Nov 29, 2019

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 sharksandcupcakes/html-inspector#1

Open

snyk-bot mentioned this pull request Dec 18, 2019

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 BeeTech-global/generator-api-ecma6#5

Open

snyk-bot mentioned this pull request Mar 8, 2020

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 michelvankessel/colored-coins-copay-wallet#1

Open

This was referenced Mar 24, 2020

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 DeviaVir/zenbot#2006

Merged

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 DeviaVir/zenbot#2010

Merged

snyk-bot mentioned this pull request Apr 3, 2020

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 telegraph/api-responder#2

Open

This was referenced Apr 24, 2020

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 electrode-io/electrode-gulp-helper#3

Open

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 electrode-io/electrode-electrify#31

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `-q` (quiet) option to `push`, `popd`, `dirs` functions. #777

Added `-q` (quiet) option to `push`, `popd`, `dirs` functions. #777

alexreg commented Oct 4, 2017

nfischer left a comment

nfischer Oct 6, 2017

alexreg Oct 6, 2017

nfischer Oct 8, 2017

nfischer Oct 6, 2017

alexreg Oct 6, 2017

alexreg commented Oct 6, 2017

nfischer left a comment

nfischer Oct 8, 2017

alexreg Oct 8, 2017

nfischer commented Oct 8, 2017

alexreg commented Oct 8, 2017

alexreg commented Oct 8, 2017

nfischer left a comment

nfischer Oct 9, 2017

nfischer commented Oct 9, 2017

alexreg commented Oct 9, 2017 via email

nfischer commented Oct 9, 2017

alexreg commented Oct 10, 2017

codecov-io commented Oct 10, 2017 •

edited

nfischer left a comment

Added -q (quiet) option to push, popd, dirs functions. #777

Added -q (quiet) option to push, popd, dirs functions. #777

Conversation

alexreg commented Oct 4, 2017

nfischer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreg commented Oct 6, 2017

nfischer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfischer commented Oct 8, 2017

alexreg commented Oct 8, 2017

alexreg commented Oct 8, 2017

nfischer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfischer commented Oct 9, 2017

alexreg commented Oct 9, 2017 via email

nfischer commented Oct 9, 2017

alexreg commented Oct 10, 2017

codecov-io commented Oct 10, 2017 • edited

Codecov Report

nfischer left a comment

Choose a reason for hiding this comment

Added `-q` (quiet) option to `push`, `popd`, `dirs` functions. #777

Added `-q` (quiet) option to `push`, `popd`, `dirs` functions. #777

codecov-io commented Oct 10, 2017 •

edited