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

Implement Text/replace #181

Merged
merged 4 commits into from
Oct 25, 2020
Merged

Conversation

basile-henry
Copy link
Collaborator

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 dhall-lang/dhall-lang#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 basile-henry added the standard-compliance Something doesn't work according to the dhall standard label Oct 16, 2020
@basile-henry basile-henry self-assigned this Oct 16, 2020
@basile-henry
Copy link
Collaborator Author

I have made a PR to change the standard: dhall-lang/dhall-lang#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. 😄

dhall/src/builtins.rs Outdated Show resolved Hide resolved
dhall/src/builtins.rs Outdated Show resolved Hide resolved
@Nadrieril
Copy link
Owner

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
Copy link
Collaborator Author

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

@basile-henry basile-henry force-pushed the text-replace branch 2 times, most recently from a0c9a3f to 539e44f Compare October 18, 2020 15:44
dhall/src/builtins.rs Outdated Show resolved Hide resolved
dhall/src/builtins.rs Outdated Show resolved Hide resolved
@Nadrieril
Copy link
Owner

Code looks great :) Just some suggestions left

@Nadrieril
Copy link
Owner

Nadrieril commented Oct 18, 2020

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
Copy link
Collaborator Author

Great! 😄 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
Copy link
Collaborator Author

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 😄

@basile-henry basile-henry merged commit 8f15b9a into Nadrieril:master Oct 25, 2020
@basile-henry basile-henry deleted the text-replace branch October 25, 2020 17:31
@Nadrieril
Copy link
Owner

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

@basile-henry
Copy link
Collaborator Author

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard-compliance Something doesn't work according to the dhall standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants