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

binary: Accepts piped input from stdin. Fixes #448 #505

Closed
wants to merge 1 commit into from

Conversation

mrjoelkemp
Copy link
Member

Issues

  • no output when the file being cat'd is blank
  • we should document the usage of this feature in the readme

Fixes #448

@@ -62,7 +62,7 @@ module.exports = function(program) {
return returnArgs;
}

if (args.length === 0) {
if (args.length === 0 && ! program._stdin) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No space for unary operators.

@mikesherov
Copy link
Contributor

@markelog, can you review this one? The CLI stuff is out of my area of expertise.

@mrjoelkemp
Copy link
Member Author

@mikesherov @markelog I opted for the old style of processing stdin, similar to how CSS-Guard does it.

Added more tests to ensure that all is well.

@mikesherov
Copy link
Contributor

@markelog any chance you can review this soon?

@mdevils
Copy link
Member

mdevils commented Jul 18, 2014

I can check this out at monday.

@mikesherov
Copy link
Contributor

@mdevils did you get a chance to review this? I'd like to put out 1.5.10 at the end of the week and would love for this to get in.

@@ -26,4 +28,19 @@ program
.option('', '\t /path/to/my-reporter\t\t(absolute path without extension)')
.parse(process.argv);

cli(program);
if (process.argv.length <= 2) {
Copy link
Member

Choose a reason for hiding this comment

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

Very strange condition. Explain this please.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably be if (process.argv.length === 2).

This triggers the piped stdin processing if jscs is invoked via node jscs, as in, the jscs command is invoked without arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mdevils Modified the condition appropriately.

@mikesherov
Copy link
Contributor

Actually, is checking for lack of arguments the only time we want stdin? What about the situation where someone does, for example: cat myfile.js | jscs --color ?

JSCS should check stdin whenever a set of files aren't specified.

@markelog
Copy link
Member

I would suggest put all logic to cli module - check if we have program.args === 0 if true than check stdin, we could even use something like this for it, although its not that complicated to do it ourselves

@markelog
Copy link
Member

process.stdin.isTTY much better :-)

@mrjoelkemp
Copy link
Member Author

@mikesherov @markelog @mdevils Made the adjustments. My only concern is that the added tests are slower (they're yellow in color) since they use exec to simulate real command line usage.

Great tip on process.stdin.isTTY, Mike.

@mikesherov
Copy link
Contributor

@mrjoelkemp LGTM to land. @markelog I'll wait for you or @mdevils to land it tomorrow.

@markelog
Copy link
Member

are slower (they're yellow in color) since they use exec to simulate real command line usage

We can improve that later

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stdin support
4 participants