Added -q (quiet) option to push, popd, dirs functions.
#777
Conversation
nfischer
left a comment
nfischer
left a comment
There was a problem hiding this comment.
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.
|
|
||
| options = common.parseOptions(options, { | ||
| 'c': 'clear', | ||
| 'q': 'quiet', |
There was a problem hiding this comment.
This doesn't seem right. In my zsh, dirs does not support the -q option:
❯ dirs -q
zsh: bad option: -qI only see -c, -l, -p, -v for this command.
There was a problem hiding this comment.
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.
zsh has the -q flag for pushd and popd. dirs does not have this flag to my knowledge.
src/dirs.js
Outdated
|
|
||
| _dirStack = dirs; | ||
| return _dirs(''); | ||
| return _dirs(options.quiet ? 'q' : ''); |
There was a problem hiding this comment.
I think you want '-q', not 'q'.
|
@nfischer Okay, have a look now, I think things should be in order. :-) |
nfischer
left a comment
nfischer
left a comment
There was a problem hiding this comment.
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
| t.truthy(shell.error()); | ||
| }); | ||
|
|
||
| test('quiet mode', t => { |
There was a problem hiding this comment.
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.
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 |
| test('quiet mode off', t => { | ||
| try { | ||
| shell.config.silent = false; | ||
| shell.pushd('test/resources/pushd'); |
There was a problem hiding this comment.
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.
|
nfischer
left a comment
nfischer
left a comment
There was a problem hiding this comment.
No problem. That's done now.
Thanks!
3 participants
Resolves issue #753.
No unit tests added yet; I'm not familiar with the framework, and not sure how best to do them.