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.

Sign up for GitHub

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

Jump to bottom

[Request] Change -in to - #26

Closed
Suavesito-Olimpiada opened this issue Aug 24, 2022 · 3 comments · Fixed by #28
Closed

[Request] Change -in to - #26

Suavesito-Olimpiada opened this issue Aug 24, 2022 · 3 comments · Fixed by #28
Assignees
@braydonk
Milestone
v0.3.0

Comments

@Suavesito-Olimpiada
Copy link

Suavesito-Olimpiada commented Aug 24, 2022 •
edited

First of, thanks for the amazing tool! :D

In many CLI programs is usual that stdin is configured automatically for input if no arguments were added. But, in many others (were no arguments means print help) it is (almost) standard to have - mean read from stdin.

I know is not important, but I propose follow the semantics of others, as I think allow for a (a little bit) more seamless experience. :)
@braydonk
Copy link
Collaborator

Thank you for the suggestion! It didn't occur to me during development to do this, but it definitely makes sense. I'll target it for v0.3.0.

@braydonk braydonk added this to the v0.3.0 milestone Aug 24, 2022
@braydonk braydonk self-assigned this Aug 24, 2022
@braydonk braydonk mentioned this issue Aug 24, 2022
Merged
@braydonk
Copy link
Collaborator

@Suavesito-Olimpiada I opened a PR for this change. Please let me know if this matches what you had in mind!
I had to do it a bit weirdly to keep consistent behaviour on Windows, since I assume - wouldn't be an alias to /dev/stdin on that.

@Suavesito-Olimpiada
Copy link
Author

Suavesito-Olimpiada commented Aug 24, 2022 •
edited

Yeah, that looks exactly like it. I understand why you make it that way, /dev/stdin does not exist properly on Windows and currently yamlfmt will not output anything if passed /dev/stdin on Linux (although it "works" in the sense that it does not crash).

braydonk added a commit to braydonk/yamlfmt that referenced this issue Aug 25, 2022
@braydonk
fix: restore functionality of -in flag
9b5f5bf 
Originally I had changed the read from stdin to be specified by a single
- argument or /dev/stdin by suggestion from google#26. In that PR I had
removed the -in flag. After sleeping on it I realized I might as well
keep that flag, since it might intuitively make sense to more people,
makes more sense on Windows, and maintains backwards compatibility (even
though that is currently not a promise of the project, might as well do
it if it's easy).
@braydonk braydonk mentioned this issue Aug 25, 2022
Merged
braydonk added a commit that referenced this issue Aug 25, 2022
@braydonk
Restore in flag (#33)
9aa43f4 
* fix: restore functionality of -in flag

Originally I had changed the read from stdin to be specified by a single
- argument or /dev/stdin by suggestion from #26. In that PR I had
removed the -in flag. After sleeping on it I realized I might as well
keep that flag, since it might intuitively make sense to more people,
makes more sense on Windows, and maintains backwards compatibility (even
though that is currently not a promise of the project, might as well do
it if it's easy).

* docs: add -in flag back to README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@braydonk braydonk

Labels
None yet
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging a pull request may close this issue.

feat: accept - or /dev/stdin instead of -in braydonk/yamlfmt
2 participants
@Suavesito-Olimpiada @braydonk