Skip to content

Use an Ordered Set for Project #670

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

Merged
merged 4 commits into from
Nov 6, 2018

Conversation

DanBurton
Copy link
Contributor

Addresses #664

Copy link
Collaborator

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

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

Looks great! Just one nitpick about naming 🙂

-- as well as a novel "difference" function.
-- Any other Set-like or List-like functionality
-- should be obtained through toSet and toList, respectively.
module Dhall.OSet (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to call this module either Dhall.Set or Dhall.OrderedSet (and similarly rename the type to either Set or OrderedSet)? The main reason is that I try to avoid one-letter abbreviations when naming types and modules. I would slightly prefer Dhall.Set for consistency with Dhall.Map (the module for ordered Maps) and also because then if you wanted the single character name you could import Dhall.Set as O and use O.Set or whatever prefix you prefer.

@Gabriella439
Copy link
Collaborator

Oh, one more thing: we should probably add a formatting test to tests/Format.hs to verify that formatting now preserves order for field projection

@DanBurton
Copy link
Contributor Author

No problem, I'll see if I can get around to making those changes this weekend.

@DanBurton
Copy link
Contributor Author

Whilst attempting the rename, I'm encountering a baffling error with the mutually recursive modules Dhall.Core and Dhall.Pretty.Internal. The latter claims to not know about Expr constructors supplied by the former. (See the failed hydra job; I'm getting the same errors locally but pushed the commit to see if it was specific to my build process. It's not.) I'm not sure how to proceed.

@ocharles
Copy link
Member

ocharles commented Nov 6, 2018

I'm not entirely sure why Dhall.Pretty.Internal is doing {-# SOURCE #-} import of Dhall.Core, but that doesn't seem necessary. Does it work if you remove that pragma? I actually can't see how it could ever have worked, as the .hs-boot doesn't have the constructors!

@DanBurton
Copy link
Contributor Author

@ocharles ah, yes it does! And that makes sense. Only one of the two needs to do the SOURCE import, to break the cycle. I'm not sure why it worked in the first place, but I did find it odd that the most innocuous, unrelated changes seemed to cause it to break.

@Gabriella439 Gabriella439 merged commit 7aba791 into dhall-lang:master Nov 6, 2018
@Gabriella439
Copy link
Collaborator

@DanBurton: Thank you! 🙂

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

Successfully merging this pull request may close these issues.

3 participants