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.

Sign up for GitHub

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

Jump to bottom

Fix diff so that an empty list is equal to itself #740

Merged
merged 2 commits into from
Dec 7, 2018
Merged

Fix diff so that an empty list is equal to itself #740

merged 2 commits into from
Dec 7, 2018

Conversation

madjar
Copy link
Contributor

@madjar madjar commented Dec 7, 2018

While using dhall diff, I realized that empty lists are considered to be different. This PR contains two commits:

  • The first one adds a quickcheck test to ensure an expression is always the same as itself (as long as it typechecks ; things go weird when you have an invalid expression, I guess because Diff assume the expression is well-formed).
  • The second one is the actual fix.

Sadly, the test required me to expose some previously private part of Dhall.diff. If that's a problem, feel free to cherry-pick the second commit.

madjar added 2 commits December 7, 2018 15:52
@madjar
Test with QuickCheck that an expression is always the same as itself
1b75022 
I noticed cases where the diffing code returns that an expression is not the
same as itself (for example, an empty list). This commit adds a QuickCheck test
to illustrate it, and maybe other cases.

Sadly, I had to expose some internals from Dhall.Diff for the test, which makes
the interface less nice.
@madjar
Fix diff so that an empty list is the same as itself
ffc7a82
basile-henry
basile-henry approved these changes Dec 7, 2018
View reviewed changes
Copy link
Collaborator

@basile-henry basile-henry left a comment

Choose a reason for hiding this comment

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

Well spotted 👍

@madjar
Copy link
Contributor Author

madjar commented Dec 7, 2018

The story behind that: I'm converting a complex json file to dhall. To ensure nothing is lost, I converted it to dhall with https://gist.github.com/madjar/252c517644c0e13ef28a2a7ca71f5fa4, and I'm comparing every change against the original with dhall diff. I was a bit distressing to get an enormous diff when passing twice the same file :)

@basile-henry
Copy link
Collaborator

@madjar I should have tested it a bit better, empty lists are a fairly obvious edge case. Great to see some more property based tests! 😄

@basile-henry
Copy link
Collaborator

Actually, could you also fix the diff for Text? It'll have the same issue but on empty strings.

dhall-haskell/dhall/src/Dhall/Diff.hs

Line 309 in ffc7a82

allDifferent = not . any isBoth

Maybe this could do with some refactoring to not duplicate too much code, up to you!

@Gabriella439 Gabriella439 merged commit f308ca2 into dhall-lang:master Dec 7, 2018
@Gabriella439
Copy link
Collaborator

@madjar: Thank you!

@madjar
Copy link
Contributor Author

madjar commented Dec 7, 2018

@basile-henry I don't see the issue with Text

> let x = TextLit (Chunks [] "") in same (diffExpression x x)
True

@madjar madjar deleted the diff-empty-list branch December 7, 2018 17:08
@Gabriella439 Gabriella439 mentioned this pull request Aug 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@basile-henry basile-henry basile-henry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@madjar @basile-henry @Gabriella439