-
Notifications
You must be signed in to change notification settings - Fork 211
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
dhall: add --output options to dhall #1399
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers! I have a few comments, but this mostly looks very good! :)
dhall/src/Dhall/Main.hs
Outdated
f (Just file) = Output file | ||
|
||
p = Options.Applicative.strOption | ||
( Options.Applicative.short 'o' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far dhall
has largely refrained from using short
options – I'm not sure why though. Probably we want the CLI commands to be as readable as possible too…
Thoughts, @Gabriel439?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the use of only long options is intentional.
In my experience, short options are an impediment to learning to beginners (especially if they learn from tutorial material that uses short options or by observing other people using short options). Short options also tend to lead to people cargo-culting the options blindly without thinking about what they do. This comment accurately summarizes what happens when short options become idiomatic:
dhall/src/Dhall/Main.hs
Outdated
case output of | ||
StandardOutput -> render annotatedExpression System.IO.stdout | ||
OutputFile file_ -> | ||
System.IO.withFile file_ System.IO.WriteMode $ render annotatedExpression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry for the misunderstanding. I thought of something like
System.IO.withFile file_ System.IO.WriteMode $ render annotatedExpression | |
System.IO.withFile file_ System.IO.WriteMode $ \h -> render h annotatedExpression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you! :)
As requested in #1386.