Skip to content
This repository has been archived by the owner on Mar 23, 2024. It is now read-only.

CLI: Only read stdin if there were no files supplied and not in tty mode. #567

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions lib/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ module.exports = function(program) {
return returnArgs;
}

if (args.length === 0 && process.stdin.isTTY) {
if (!args.length && process.stdin.isTTY) {
console.error('No input files specified. Try option --help for usage information.');
defer.reject(1);

Expand Down Expand Up @@ -106,8 +106,12 @@ module.exports = function(program) {
checker.registerDefaultRules();
checker.configure(config);

if (! process.stdin.isTTY) {
// Handle usage like cat myfile.js | jscs
// Handle usage like 'cat myfile.js | jscs' or 'jscs -''
var usedDash = args[args.length - 1] === '-';
if (!args.length || usedDash) {
// So the dash doesn't register as a file
if (usedDash) { args.length--; }

process.stdin.setEncoding('utf8');

process.stdin.on('data', function(chunk) {
Expand Down
30 changes: 29 additions & 1 deletion test/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ describe('modules/cli', function() {
});
});

it('should should accept empty input being piped', function(done) {
it('should accept empty input being piped', function(done) {
// 'cat myEmptyFile.js | jscs' should report a successful run
rAfter();

Expand All @@ -233,6 +233,34 @@ describe('modules/cli', function() {
done();
});
});

it('should not accept piped input if files were specified (#563)', function() {
var checker = require('../lib/checker');
var spy = sinon.spy(checker.prototype, 'checkPath');
Copy link
Member

Choose a reason for hiding this comment

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

Just as in test below you need to restore the method you are spying on – see "Spying on existing methods" in here

Copy link
Member Author

Choose a reason for hiding this comment

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

Restoring this method works well. Good catch. Done.


var result = cli({
args: [__dirname + '/data/cli/success.js']
});

return result.promise.always(function() {
assert(spy.called);
checker.prototype.checkPath.restore();
rAfter();
});
});

it('should check stdin if - was supplied as the last argument (#563)', function() {
var spy = sinon.spy(process.stdin, 'on');
Copy link
Member Author

Choose a reason for hiding this comment

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

@markelog Restoring process.stdin.on is problematic. I'm not 100% sure why (still trying to find the cause through sinon's source), but process.stdin.on.restore only exists until the first call to process.stdin.on at https://github.com/mrjoelkemp/node-jscs/blob/fix_stdin/lib/cli.js#L117. After this call, process.stdin.on.restore is gone. Hence, adding a restore call in the promise callback throws an error.

Copy link
Member

Choose a reason for hiding this comment

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

hm, it's already restored, by the time the test is done, so we don't need to do that ourselves


var result = cli({
args: [__dirname + '/data/cli/success.js', '-']
});

return result.promise.always(function() {
assert(spy.called);
rAfter();
});
});
});

describe('reporter option', function() {
Expand Down