-
Notifications
You must be signed in to change notification settings - Fork 213
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
Add freeze command #486
Add freeze command #486
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.
Tests are not required but I would like to see a Tutorial
section addition. Even just turning your pull request description into a new tutorial section would be good enough
src/Dhall/Main.hs
Outdated
@@ -139,6 +142,19 @@ parseMode = | |||
parserWithHelper = Options.Applicative.helper <*> parser | |||
parser = Format <$> optional parseInplace | |||
|
|||
freezeSubcommand = |
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 think you can simplify this as:
freezeSubcommand = subcommand "freeze" "Add hashes to all import statements of an expression" parseFreeze
where
parseFreeze = Freeze <$> optional parseInplace
where | ||
fromStdin = System.IO.hSetEncoding System.IO.stdin System.IO.utf8 >> Data.Text.IO.getContents | ||
|
||
hashImport :: Import -> IO Import |
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 think you should also export this utility since it is handy for people using the Haskell API
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.
Done.
Added a paragraph on freeze to the Tutorial, please take a look. |
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.
Just two minor suggestions
I also added you as a contributor so you can merge at any time
src/Dhall/Tutorial.hs
Outdated
-- | ||
-- If you have a file which either doesn't already use hashed imports, | ||
-- or you changed some of the imports and want to update the hashes you can use the | ||
-- freeze command to either add or updated hashes: |
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.
s/updated/update/
src/Dhall/Tutorial.hs
Outdated
-- | ||
-- > cat foo.dhall | ||
-- '' | ||
-- let replicate = ./r.dhall |
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.
Maybe use the GitHub URL for the import so that users can reproduce the example and also so that I can later add the example to the test suite
The freeze command .. - takes input from stdin or file - traverses the Expression adding hashes to imports - writes result to stdout or file
This PR adds a
freeze
command as described in #437Given
the freeze command will produce:
The PR doesn't include tests or updates to
Tutorial.hs
- can those be done separately or should those be included to make this eligible for merge?Thanks to @Gabriel439 and @ocharles for the help on this.