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

Union Access #657

Merged
merged 21 commits into from
Oct 26, 2018
Merged

Union Access #657

merged 21 commits into from
Oct 26, 2018

Conversation

FintanH
Copy link
Collaborator

@FintanH FintanH commented Oct 25, 2018

Addressing phase 1 of dhall-lang/dhall-lang#244 and #635

@Gabriel439: I haven't implemented any tests for this and I should but is there documentation of how the tests are laid out? I'm slightly confused but possibly it'd be fine if I spent some more time looking through them :)

@f-f
Copy link
Member

f-f commented Oct 25, 2018

@FintanH there is some info about how tests are structured in dhall-lang/dhall-lang#240.
I'm writing some of that down in a file to keep in that repo (because we'll have "assurance tests" there)

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 25, 2018

@f-f have I told you you're a hero? Thank you. I think I can see what's going on now :)

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 25, 2018

Is there something I have to do when adding new files? It seems the tests couldn't find the new files, but I added them :/

@f-f
Copy link
Member

f-f commented Oct 25, 2018

@FintanH yes, you should add tests/normalization/tutorial/access/* somewhere around here

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 25, 2018

@f-f ahhh of course 😅

@@ -3273,22 +3293,70 @@ prettyTypeMessage (NotARecord lazyText0 expr0 expr1) = ErrorMessages {..}
\ Invalid: Not a record \n\
\ \n\
\ \n\
\────────────────────────────────────────────────────────────────────────────────\n\
\ \n\
\You tried to access the field:aaa \n\
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
\You tried to access the field:aaa \n\
\You tried to access the field: \n\

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hahaha whooops!

Copy link
Member

@f-f f-f 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 👍

Let's also add some typecheck tests, since we're adding code paths in there as well

@FintanH
Copy link
Collaborator Author

FintanH commented Oct 25, 2018

@f-f Ya I thought about that, but didn't see any existing ones that had similar functionality. But that's a bad excuse really 😳

\ \n\
\ ┌───────────────────────────────────────────┐ \n\
\ │ λ(r : { foo : Bool, bar : Text , baz : Natural }) → r.{ foo, bar } │ ... and so is this \n\
\ └───────────────────────────────────────────┘ \n\
Copy link
Member

Choose a reason for hiding this comment

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

Is this boxing correct? It doesn't render correctly, but that could be my browser (I use a font with ligatures that might be messing things up, though everything else looks correct.

@FintanH FintanH merged commit 8cad992 into dhall-lang:master Oct 26, 2018
@FintanH FintanH deleted the fintan/union-access branch October 26, 2018 10:07
@jneira jneira mentioned this pull request Nov 22, 2018
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.

None yet

4 participants