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
Added -q
(quiet) option to push
, popd
, dirs
functions.
#777
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -173,6 +178,7 @@ function _dirs(options, index) { | |||
|
|||
options = common.parseOptions(options, { | |||
'c': 'clear', | |||
'q': 'quiet', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zsh
has the -q
flag for pushd
and popd
. dirs
does not have this flag to my knowledge.
src/dirs.js
Outdated
@@ -95,7 +97,7 @@ function _pushd(options, dir) { | |||
} | |||
|
|||
_dirStack = dirs; | |||
return _dirs(''); | |||
return _dirs(options.quiet ? 'q' : ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want '-q'
, not 'q'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@nfischer Okay, have a look now, I think things should be in order. :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
test/popd.js
Outdated
@@ -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 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also test stdout
for the case where -q
is not passed? The test should have the same layout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost did this before, but wasn't sure if you'd want them. Done now.
Oh, this needs docs generated. Please run |
Yep, those were my thoughts too. |
That's all done now. Word of warning: the existing code for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % comment. Thanks!
test/popd.js
Outdated
try { | ||
shell.config.silent = false; | ||
shell.pushd('test/resources/pushd'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
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.
|
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 |
No problem. That's done now. |
Codecov Report
@@ Coverage Diff @@
## master #777 +/- ##
==========================================
+ Coverage 95.39% 95.39% +<.01%
==========================================
Files 33 33
Lines 1237 1239 +2
==========================================
+ Hits 1180 1182 +2
Misses 57 57
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. That's done now.
Thanks!
Resolves issue #753.
No unit tests added yet; I'm not familiar with the framework, and not sure how best to do them.