search

dhall-lang / dhall-to-cabal

Compile Dhall expressions to Cabal files
MIT License
100 stars 19 forks source link

CabalToDhall: implement unifyCondTree using associativity. #54

Closed quasicomputational closed 6 years ago

quasicomputational commented 6 years ago

I think that this is a lot clearer about what it is doing, rather than the previous algorithm, which was going wrong and causing #36 and then making my eyes cross as I tried to figure out precisely how.

Fixes #36.

I'm not 100% confident that this is correct since it doesn't round trip (see #53) and the test's output is too big to inspect directly, but I'm not sure where a bug could be hiding in the code.

ocharles commented 6 years ago

Wow, terrific - this looks much simpler. I'll try and get to this soon (along with your other PRs!)

quasicomputational commented 6 years ago

OK, now that #53 is fixed, I've verified that the test case can round-trip, so I've got some confidence in this PR.

ocharles commented 6 years ago

I've had another read, and this both way simpler and seems to make sense. You walk the CondTree and cat all branches at each level into a single branch essentially. Given that all the tests pass, I'm happy with this. Good to merge?

quasicomputational commented 6 years ago

I'm happy with it!