Skip to content
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-haskell → dhallj #217

Merged
merged 2 commits into from
Apr 23, 2020
Merged

dhall-haskell → dhallj #217

merged 2 commits into from
Apr 23, 2020

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Apr 14, 2020

With dhallj, we no longer depend on dhall-haskell and its binaries, in order to only depend on a JVM.

Two one minor downside:

  • No formatting check right now, but this might change.
  • The generated YAML has ugly multi-line strings, and I think that is a limitation of SnakeYAML (see here). This is a known issue in dhallj, will probably be fixed soon. Fixed in dhallj 0.2.0.

Just run convertDhall for updating the YAML files.

@rossabaker
Copy link
Member

Eliminating the curl calls in the build is a big plus, as it also means simplifying (build) developer environments. This is good when it's ready.

@amesgen
Copy link
Member Author

amesgen commented Apr 21, 2020

This PR should now be ready. I updated the PR description.

@amesgen amesgen marked this pull request as ready for review April 21, 2020 11:48
@@ -24,7 +24,7 @@ let steps =
c.Run::{
, name = "Test docs"
, run = "docs/makeSite"
, if = Some "startsWith(matrix.scala, '2.12')"
, `if` = Some "startsWith(matrix.scala, '2.12')"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These backticks are necessary as the previous version was only accepted due to a bug in dhall-haskell.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@rossabaker rossabaker merged commit 5e36ce7 into http4s:master Apr 23, 2020
@amesgen amesgen deleted the use-dhallj branch April 23, 2020 01:32
@amesgen
Copy link
Member Author

amesgen commented Apr 23, 2020

Ah, release.yml still has one issue, " is escaped to ".

@amesgen
Copy link
Member Author

amesgen commented Apr 23, 2020

Ok, this is in fact a dhallj issue, I will report it (see travisbrown/dhallj#56). I personally don't think it is worth to temporarily revert this PR, as this does not seem like a major issue.

@travisbrown
Copy link

Sorry about these escaping issues—the YAML export clearly needs some attention and tests, which I should be able to get to in the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants