Skip to content

Read from stdin #605

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

Closed
wants to merge 3 commits into from
Closed

Read from stdin #605

wants to merge 3 commits into from

Conversation

pluma
Copy link

@pluma pluma commented Jul 28, 2015

This probably needs testing, but I couldn't figure out how to add CLI tests and make sure they pass for each environment.

I've implemented readStdin for Node, Rhino and WSH with the latter two being based on various online tutorials -- I have only verified the Node implementation.

As the internals always expect a filename, the filename is passed as "<stdin>" when reading from stdin. I've tried to keep the changes minimal, but reading from stdin in Node is always async, so I had to introduce a callback.

@pluma pluma mentioned this pull request Jul 28, 2015
@pluma
Copy link
Author

pluma commented Jul 28, 2015

For the record: the Rhino code seems to work (I verified the stdin implementation by adding prints), but CSSLint's formatter hangs for me even without this PR (it doesn't work with file name arguments either -- maybe I'm doing it wrong).

@Arcanemagus
Copy link
Contributor

Any chance that this could be merged or at least reviewed? I would absolutely love to get rid of AtomLinter/csslint and switch AtomLinter/linter-csslint back to this official repository now that it seems to be active again, however without this PR I can't do that yet.

If this PR has issues @steelbrain or I can throw together one with the implementation we went with over in the AtomLinter fork which has been working well for our users at least.

@pluma
Copy link
Author

pluma commented Jan 9, 2016

Should I attempt to rebase this? I'd be willing to look into it if there is any intention to ever merge this PR.

@XhmikosR
Copy link
Member

XhmikosR commented Feb 3, 2016

@pluma: yes, please fetch and rebase.

@pluma
Copy link
Author

pluma commented Feb 3, 2016

@XhmikosR I've rebased to upstream master and fixed the conflicts. npm test is passing on my machine and travis. Piping files to dist/cli.js behaves as expected, filename is given as <stdin>.

@XhmikosR
Copy link
Member

XhmikosR commented Feb 3, 2016

@pluma: can you add 1-2 tests for this?

@pluma
Copy link
Author

pluma commented Feb 3, 2016

@XhmikosR any idea how I can test piping files to it?

@XhmikosR
Copy link
Member

XhmikosR commented Feb 6, 2016

@pluma: sorry, I don't know about that.

@sergeychernyshev
Copy link
Contributor

@pluma can we package a sample CSS file with it and do piping using child_process or something alike?

@pluma
Copy link
Author

pluma commented Feb 26, 2016

@sergeychernyshev sure, if you can make any sense of the tests folder and tell me how I can add a test case like that, I'm all in. I just can't justify putting much time into this PR anymore.

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

Successfully merging this pull request may close these issues.

5 participants