Skip to content
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

Read from stdin #82

Merged
merged 8 commits into from
Dec 14, 2015
Merged

Read from stdin #82

merged 8 commits into from
Dec 14, 2015

Conversation

eeue56
Copy link
Contributor

@eeue56 eeue56 commented Dec 12, 2015

Outfoxes #66

I also changed the spec slightly -

  • If elm-format - run, then read from stdin
  • If elm-format --stdin run, then read from stdin
  • If --output is set, then save to that output file

Ideally, I think I'd like it so that there was only a --stdin since it's possible to accidentally do - and be trapped in a stdin reader. I added the --stdin option, so that it self documents the ability to automatically it print it out when the file is run with no args or --help is set.

The only thing I haven't done is update the docs as part of this PR - I wanted to check with @avh4 whether we want to keep elm-format - as a thing or to ditch it first.

@avh4
Copy link
Owner

avh4 commented Dec 13, 2015

Okay, I'm sold on removing elm-format - in favor of --stdin. I think that fits well with Elm's philosophy of preferring well-documented interfaces.

Also, there is currently behavior where if multiple files are specified, then it is an error to give --output. We should handle this similarly: if --stdin is given, then it should be an error to provide any other filenames (and show an appropriate message).

Are there any other edge-cases we need to consider?

@eeue56
Copy link
Contributor Author

eeue56 commented Dec 14, 2015

Okay, done and done - this makes the code a lot cleaner, so I'm happy with this solution. As it stands, I don't think there's any other edge cases. There might be when more support for tooling is worked on, because of how the tooling wants to work, but I can't think of any right now.

@avh4 avh4 merged commit 36c725e into avh4:master Dec 14, 2015
@avh4
Copy link
Owner

avh4 commented Dec 14, 2015

Looks great, thank you!

This was referenced Dec 14, 2015
@eeue56 eeue56 deleted the read-from-stdin branch December 14, 2015 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants