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

Fix dhall freeze to better handle protected imports #1772

Merged
merged 4 commits into from
May 5, 2020

Conversation

Gabriella439
Copy link
Collaborator

This includes two changes:

  • dhall freeze will now fix an ordinary protected import that does not
    yet have a fallback import
  • dhall freeze will now add a fallback import to an import that is
    already protected

This includes two changes:

* `dhall freeze` will now fix an ordinary protected import that does not
  yet have a fallback import
* `dhall freeze` will now add a fallback import to an import that is
  already protected
@sjakobi
Copy link
Collaborator

sjakobi commented May 3, 2020

@Gabriel439 Do the two changes from the PR description apply only with the --cache flag or in general?

@Gabriella439
Copy link
Collaborator Author

@sjakobi: These changes only affect the --cache flag. The only thing that is modified is the cache function, which is only used to rewrite an expression if the --cache flag is enabled

Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

LGTM, although I'm not confident that I haven't missed anything. I believe we also don't have any tests. Maybe @SiriusStarr could give this a spin?

Comment on lines +161 to +162
-- This case is necessary because `transformOf` is a bottom-up
-- rewrite rule. Without this rule, if you were to transform a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tangential: Are there any top-down rewrite rules that we could use instead? I believe we have to apply similar workarounds in other places.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any top-down rewrite rules that we could use instead?

I assume the types probably wouldn't work out…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There aren't any that I know of.

dhall/src/Dhall/Freeze.hs Outdated Show resolved Hide resolved
Gabriella439 and others added 2 commits May 4, 2020 07:52
@Gabriella439
Copy link
Collaborator Author

@sjakobi: I went ahead and added tests to confirm that this behaves as expected, so I'll go ahead and mege

@mergify mergify bot merged commit 3b653f8 into master May 5, 2020
@mergify mergify bot deleted the gabriel/freeze_fixes branch May 5, 2020 03:18
@sjakobi
Copy link
Collaborator

sjakobi commented May 5, 2020

Thank you for adding tests, @Gabriel439! :)

@Gabriella439
Copy link
Collaborator Author

@sjakobi: You're welcome! 🙂

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

Successfully merging this pull request may close these issues.

2 participants