-
Notifications
You must be signed in to change notification settings - Fork 211
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
Small fixes to dhall diff
#1263
Conversation
Related to #1255 * Simplify function type diffs by omitting the bound variable name when possible * Non-zero exit code when `dhall diff` is non-empty Note that this is a breaking change to the `Dhall.Diff` API by changing the exposed utilities to all expose the more general `Diff` type instead of a `Doc`. This means that we also no longer need separate exports for `diff` and `diffExpression`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Simplify function type diffs by omitting the bound variable name when possible
- Simplify diffs of bound variables when possible
The appList
test case seems to cover the second change. Would you mind also adding a test for the first one? My current general impression is that we have very low test coverage for some fairly tricky code here.
dhall/src/Dhall/Diff.hs
Outdated
where | ||
Diff {..} = diffExpression l0 r0 | ||
diff :: (Eq a, Pretty a) => Expr Void a -> Expr Void a -> Diff | ||
diff = diffExpression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about renaming diffExpression
to diff
?
... as suggested by @sjakobi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers! :)
Related to #1255
dhall diff
is non-emptyNote that this is a breaking change to the
Dhall.Diff
API by changing theexposed utilities to all expose the more general
Diff
type instead of aDoc
. This means that we also no longer need separate exports fordiff
anddiffExpression
.