-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
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 format
to the CLIgren.json
to find the source directories to format.gren/
,node_modules/
, and.git/
are ignored when recursively searching directories--yes
can be used to skip the confirmation prompt--stdin
can be used to format stdin to stdout--stdin
is 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"