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

Implementation of toMap #1041

Merged
merged 14 commits into from
Jul 15, 2019
Merged

Implementation of toMap #1041

merged 14 commits into from
Jul 15, 2019

Conversation

blamario
Copy link
Contributor

I believe I'm done. A dhall-lang PR with tests will follow.

@EggBaconAndSpam
Copy link
Collaborator

EggBaconAndSpam commented Jul 8, 2019

Could you please remove the unrelated changes you accidentally included in the PR? That would make merging much easier. In particular, why did you add the Data.Monoid ((<>)) import to so many files?

edit: Never mind, the commit history tells the story here.

@Gabriella439
Copy link
Collaborator

@blamario: If you merge blamario#1 into your branch then this should be ready to merge

@blamario
Copy link
Contributor Author

@blamario: If you merge blamario#1 into your branch then this should be ready to merge

I have merged it, but tests/normalization/success/unit/ToMapWithType still fails for me. Admittedly I'm not too sure what I'm doing with the dhall-lang submodule.

@sjakobi
Copy link
Collaborator

sjakobi commented Jul 15, 2019

I have merged it, but tests/normalization/success/unit/ToMapWithType still fails for me. Admittedly I'm not too sure what I'm doing with the dhall-lang submodule.

Did you git submodule update?

@blamario
Copy link
Contributor Author

Did you git submodule update?

That was it, thanks! All tests are passing now.

@Gabriella439 Gabriella439 merged commit 8aa2ac3 into dhall-lang:master Jul 15, 2019
@Gabriella439
Copy link
Collaborator

@blamario: Thank you so much for all the work you put into this! 🙂

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.

A bit late to the party – I'll address these myself.

@@ -527,6 +528,15 @@ eval !env t =
| Just f <- Dhall.Map.lookup k m -> maybe f (vApp f) mt
| otherwise -> error errorMsg
(x, y, ma) -> VMerge x y ma
ToMap x ma -> case (evalE x, evalE <$> ma) of
(VRecordLit m, Just (VList t)) | null m ->
VListLit (Just t) (Dhall.Map.foldMapWithKey entry m)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we know that m is empty:

Suggested change
VListLit (Just t) (Dhall.Map.foldMapWithKey entry m)
VListLit (Just t) Data.Sequence.empty

(VRecordLit m, Just (VList t)) | null m ->
VListLit (Just t) (Dhall.Map.foldMapWithKey entry m)
(VRecordLit m, _) ->
VListLit Nothing (Dhall.Map.foldMapWithKey entry m)
Copy link
Collaborator

Choose a reason for hiding this comment

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

mconcatting a singleton for each element seems fairly inefficient to me. I suspect it would be better to do something like

Data.Sequence.fromList . ... . Dhall.Map.toList

)
)

let keyValues = Dhall.Map.foldMapWithKey entry kvsX
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

sjakobi added a commit that referenced this pull request Jul 15, 2019
…addressing my own comments on
#1041.
sjakobi added a commit that referenced this pull request Jul 15, 2019
…addressing my own comments on
#1041.
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

5 participants