Port to Dhall 1.23 #150
Port to Dhall 1.23 #150
Conversation
|
Status update: 1.22 has been released, but the fix for the above bug didn't make it, so this is still waiting on a release. There's also another thing I think we're being bitten by (dhall-lang/dhall-haskell#926) which we will need a fix for before upgrading. |
|
I'm happy to have this merged and released when ready. I'm on holiday until
May 15th and won't be able to release until after then. Thanks for the PR!
…On Thu, 2 May 2019, 3:52 am quasicomputational, ***@***.***> wrote:
Status update: 1.22 has been released, but the fix for the above bug
didn't make it, so this is still waiting on a release. There's also another
thing I think we're being bitten by (dhall-lang/dhall-haskell#926
<dhall-lang/dhall-haskell#926>) which we will
need a fix for before upgrading.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#150 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAFDDS5F5YVED6LYVOY6ATPTHYITANCNFSM4HI6WIEA>
.
|
quasicomputational
force-pushed
the
dhall-1.22
branch
3 times, most recently
from
e07e17e
to
d324b20
Compare
|
The 'no-user-facing changes' was nice while it lasted. In practice, this is going to mean that we'll skip 1.22 entirely and move on to 1.23 next, since the nullary thing is a breaking change and 1.22 is too broken. |
quasicomputational
force-pushed
the
dhall-1.22
branch
2 times, most recently
from
31e676c
to
d7d0695
Compare
quasicomputational
force-pushed
the
dhall-1.22
branch
from
d7d0695
to
548aad4
Compare
quasicomputational
force-pushed
the
dhall-1.22
branch
2 times, most recently
from
984ab4b
to
574997b
Compare
|
Execution seems a whole lot quicker with the new Dhall version, which is great - still not too quick to be perceptible (like hpack is), but it's no longer so slow that that alone is disqualifying. With any luck, this PR's ready to go now. |
quasicomputational
force-pushed
the
dhall-1.22
branch
2 times, most recently
from
e134c71
to
b7f2ebb
Compare
This is less code and (IMO) more idiomatic. More importantly, it will also work with both normalisation forms for union constructors without any extra work.
This is a breaking change that's semi-forced because now `genericAuto` and `UnionType` will use nullary constructors wherever they can.
quasicomputational
force-pushed
the
dhall-1.22
branch
3 times, most recently
from
8341d25
to
4a5b19e
Compare
quasicomputational
force-pushed
the
dhall-1.22
branch
from
4a5b19e
to
529dd21
Compare
|
No luck, as I might've predicted - Hydra's unhappy with something, but I don't have much to go on except that adding the |
|
No worries, I'm home Wednesday and can fix it then. Note that there is
still an in progress branch that is even faster. We're very close to
non-noticeable, I think.
…On Sun, 12 May 2019, 4:58 pm quasicomputational, ***@***.***> wrote:
No luck, as I might've predicted - Hydra's unhappy with something, but I
don't have much to go on except that adding the base-compat-batteries
override triggered it.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#150 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAFDDR3T4MLJFT53OF7F63PU7E2FANCNFSM4HI6WIEA>
.
|
lib/CabalToDhall.hs
Outdated
| Right ( Cabal.UnknownLicense "UnspecifiedLicense" ) -> | ||
| license "Unspecified" ( Expr.RecordLit mempty ) | ||
| licenseNullary "Unspecified" | ||
| Right ( Cabal.UnknownLicense l ) -> | ||
| license "Unspecified" ( Expr.TextLit (Expr.Chunks [] (StrictText.pack l)) ) |
There was a problem hiding this comment.
maybe it should be
| license "Unspecified" ( Expr.TextLit (Expr.Chunks [] (StrictText.pack l)) ) | |
| license "Unknown" ( Expr.TextLit (Expr.Chunks [] (StrictText.pack l)) ) |
There was a problem hiding this comment.
Plausible. I think it's worth keeping the meaningful changes in this PR to a minimum, though - if this code is confused, it's not as a result of the 1.23-related changes, and it can be dealt with separately.
There was a problem hiding this comment.
Agree. Maybe you can't write a cabal config in the actual version to reach that code path (so in that case maybe the union case could be removed from dhall-to-cabal)
As discussed in #150 (see comments). Setting an unknown license with cabal spec < 2.0 threw an error as License.Unspecified doesn't take a Text arg.
|
Ok, I took a slightly alternative approach of just jailbreaking something earlier. Hydra is happy now, so this is merged. Thanks! |
|
Ah, yeah, that's a good idea - I was trying to manually solve the dep tree, but cutting the knot like this is much simpler. Thanks! |
No user-facing changes in this PR, though it might be an idea to use the new streamlined enum-like union syntax where we can.
This doesn't actually work yet on two counts:
I think we're being bitten by a Dhall bug, 1.22: union construction seems not to be normalised dhall-haskell#915, which has left me unable to run the tests.
1.22 hasn't made it onto Hackage anyway, and then the Nix derivations will need to be updated as well.