-
Notifications
You must be signed in to change notification settings - Fork 9
Feature/lamdera format #39
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
base: lamdera-next
Are you sure you want to change the base?
Feature/lamdera format #39
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.
Tested locally, looking good! A few questions.
extra/Lamdera/CLI/Format.hs
Outdated
TIO.putStrLn " lamdera format src/ # format all *.elm files in the src directory\n" | ||
TIO.putStrLn "Full guide to using elm-format at <https://github.com/avh4/elm-format>" | ||
Exit.exitSuccess | ||
else case inputs of |
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.
Wondering if it was possible to patch directly into elm-format's existing handling code here rather than replicating it? Or was there some problem doing so?
2324e47
to
76cdecd
Compare
Thanks for the review @supermario! I apologize for the delayed response - it's been several months and I've had a lot on my plate. I've investigated patching directly into elm-format's CLI handling code as you suggested. While it would be ideal to reuse elm-format's CLI code directly, I found that the two CLIs have different architectures:
Directly integrating would require either:
The current implementation:
That said, if you feel strongly about deeper integration, I'm happy to explore it further. The trade-off would be more coupling with elm-format's internals vs. the current approach of depending only on its public API. |
Sounds like exactly what we’re after in this case! Though I realize it may be tricky to have it work with ‘lamdera format ….’ versus ‘elm-format ….’ I.e the extra command, but have a bash, should be possible. |
Hey @supermario ! I've implemented the special-case approach as you suggested. The implementation is minimal - just 11 lines in FormatDirect.hs to call elm-format, and a simple conditional check in Terminal.hs. |
a04a920
to
fe36504
Compare
As suggested in PR review, the format command now bypasses Lamdera's normal CLI processing and delegates directly to elm-format's mainIO. This ensures perfect compatibility with all elm-format features and reduces code duplication. Changes: - Intercept "format" command early in Terminal.hs - Add minimal FormatDirect module that calls elm-format - Remove 244-line Format.hs implementation - Keep stub command for help display consistency
fe36504
to
ab00039
Compare
Lamdera Compiler (view)
This pull request introduces a new feature to format Elm source files using the
lamdera format
command. The most important changes include adding the newLamdera.CLI.Format
module, updating existing modules to support the new command, and modifying theLamdera.CLI
module to include the new format command.New Feature: Elm Source File Formatting
Lamdera.CLI.Format
module to implement thelamdera format
command, which includes functionality to format Elm source files, handle various command-line options, and provide usage instructions.Lamdera.CLI
module to export the newformat
function and include the format command in the CLI interface. [1] [2] [3]elm.cabal
to include the newLamdera.CLI.Format
module.Codebase Enhancements
Ext.ElmFormat
module to support file path input for formatting functions and handle new imports required for formatting Elm source files. [1] [2]format
command to the list of available commands in themain
function ofterminal/src/Main.hs
.