-
-
Notifications
You must be signed in to change notification settings - Fork 29
Add format CLI subcommand #46
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
Conversation
robinheghan
left a comment
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.
This seems good to me!
I much prefer reviewing small PRs, so adding bit-by-bit like this is a good strategy, especially when it's trivial to feature toggle it like you suggest.
For code formatting I use: https://github.com/tweag/ormolu
Other than that, I too try to adapt to the surrounding code.
I made a small suggestion, but once you've considered it you're free to merge :)
Great work! 💯
builder/src/AbsoluteSrcDir.hs
Outdated
| mkAbsoluteSrcDir :: FilePath -> IO AbsoluteSrcDir | ||
| mkAbsoluteSrcDir srcDir = | ||
| AbsoluteSrcDir | ||
| <$> Dir.canonicalizePath srcDir |
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.
I have this allergy against repeating the module name in the module's functions.
I realize that the mkX pattern is probably used elsewhere in this project, but for me I think a function name like init (AbsoluteSrcDir.init) works better.
What do you think?
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.
I'm not even sure if mk* is used elsewhere here... I was just using my recollection of this article https://kowainik.github.io/posts/naming-conventions#mk
I don't really have a personal preference. init sounds good to me.
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.
Oh, init is kinda bad because it conflicts with Prelude.init.
How about AbsoluteSrcDir.fromFilePath ?
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.
fromFilePath is much better :)
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 🥳
This is just the first part of #42, adding just the CLI subcommand and not actually implementing any formatting yet.
@robinheghan Are you up for merging pieces (like this PR) before the entire formatting feature is fully working? If not, then I'll create a feature branch that these smaller PRs will merge into. (Also note, if this is merged, it's trivial to disable it just by removing it from the list of available subcommands.)
This PR includes:
gren formatto the CLIgren.jsonto find the source directories to format.gren/,node_modules/, and.git/are ignored when recursively searching directories--yescan be used to skip the confirmation prompt--stdincan be used to format stdin to stdout--stdinis used with explicit file namesI tried my best to guess what the preferred coding style is based on what's already there. Lmk if there's any trivial stuff that you want done differently.
Console "screenshots"