search

Nadrieril / dhall-rust

Maintainable configuration files, for Rust users
Other
303 stars 27 forks source link

Implement Text/replace #181

Closed basile-henry closed 3 years ago

basile-henry commented 3 years ago

This PR implements the new built-in Text/replace which is part of standard v19.0 (tracking issue #180)

There is one test that currently fails. I believe it is an issue with the way the standard is currently specified around text replacement with Text that isn't fully evaluated. My understanding is that this specific case should get fixed in a future revision of the standard https://github.com/dhall-lang/dhall-lang/pull/1065

Should I disable this Text/replace failing test? What about tests for other v19.0 features (with built-in), should I temporarily disable them until they get implemented?

basile-henry commented 3 years ago

I have made a PR to change the standard: https://github.com/dhall-lang/dhall-lang/pull/1084 This implements my suggested changes, so we should probably wait to see if it actually gets approved. If it doesn't I will update this PR to reflect whatever decision is taken. :smile:

Nadrieril commented 3 years ago

Thanks for your help, I was dreading to have to implement it myself! My comments are actually about the standard I think, so we'll have to wait for their decision. Code looks great otherwise, I can see you've become familiar with the codebase ^^

Should I disable this Text/replace failing test? What about tests for other v19.0 features (with built-in), should I temporarily disable them until they get implemented?

Yes feel free to disable tests that fail for now

basile-henry commented 3 years ago

I have reverted back to the simpler (correct) interpolation rules :smile:

Nadrieril commented 3 years ago

Code looks great :) Just some suggestions left

Nadrieril commented 3 years ago

Great! You may go ahead and merge it now, or you can wait for the corresponding dhall-lang PR to get merged if you think it makes more sense

basile-henry commented 3 years ago

Great! :smile: Yes I think I will wait for the dhall-lang PR to be accepted first, so I can point the submodule to dhall-lang master instead of my fork!

basile-henry commented 3 years ago

I have pointed the dhall-lang submodule towards the latest master now that the Text/replace revisions have been merged. I have also rebased on the latest master here. I intend on merging when CI passes 😄

Nadrieril commented 3 years ago

Perfect, thanks! Are there any other changes than the new with handling that we haven't implemented yet?

basile-henry commented 3 years ago

I think with changes are the only thing left. But I'm only looking at the changelog from dhall-lang: https://github.com/dhall-lang/dhall-lang/blob/master/CHANGELOG.md#v1900

From this changelog, other than Text/replace and with changes it looks like all the changes are prelude related, which I don't think we need to worry about.

Nadrieril commented 3 years ago

Great :) I've released 0.7.4 with this PR in