[#1392] Injecting Data.Map #1412

jiegillet · 2019-10-10T14:43:41Z

To do:

instance Inject (Data.Map.Map k v)
Clean up comments and extra empty lines
Add quicktest tests (Haskell => Dhall => Haskell == id)
~~Data.Map Text v embedded as record via newtype RecordMap~~

Question: I made two implementations (in my first two commits), one using Generic, and an explicit one. Is there a difference in efficiency? The first looks cooler to me.

dhall/src/Dhall.hs

sjakobi · 2019-10-10T15:24:04Z

Question: I made two implementations (in my first two commits), one using Generic, and an explicit one. Is there a difference in efficiency? The first looks cooler to me.

You can benchmark this if you want. I'm not sure. I believe the current version is easier to understand and should compile faster.

…e-map

sjakobi

What's up with the update to the submodule?

sjakobi · 2019-10-11T13:56:14Z

dhall/src/Dhall.hs

+        declaredOut = App List $ Record $ Dhall.Map.fromList
+                          [("mapKey", declaredK), ("mapValue", declaredV)]
+
+        mapEntries = fmap recordPair . Data.Sequence.fromList . Data.Map.toList


I have a hunch that this would be more efficient, but no proof… :/

Suggested change

mapEntries = fmap recordPair . Data.Sequence.fromList . Data.Map.toList

mapEntries = Data.Sequence.fromList . map recordPair . Data.Map.toList

sjakobi · 2019-10-11T14:00:40Z

dhall/src/Dhall.hs

+                | Data.Map.null m = Just declaredOut
+                | otherwise       = Nothing
+
+        declaredOut = App List $ Record $ Dhall.Map.fromList


So far the code base mostly uses parentheses instead of $. @Gabriel439 can probably explain why.

I think we should stick with the convention unless there's a good reason not to.

It comes from this: http://www.haskellforall.com/2015/09/how-to-make-your-haskell-code-more.html

... although I usually only apply that to my own coding style. I usually do not review other people's code for style that much

Interesting read, thank you!

jiegillet · 2019-10-11T14:12:33Z

I don't know what's up with the submodule either, I've never used one before, it keeps sneaking in and it doesn't really show in git diff... I'll get rid of it somehow.

I posted in the issue, but do you know what I can do about the RecordMap instance?

sjakobi · 2019-10-11T14:15:50Z

I don't know what's up with the submodule either, I've never used one before, it keeps sneaking in and it doesn't really show in git diff... I'll get rid of it somehow.

This should work:

$ cd dhall/dhall-lang
$ git checkout cb569fc
$ cd ../..
$ git add dhall/dhall-lang
$ git commit

I posted in the issue, but do you know what I can do about the RecordMap instance?

I've responded in the issue.

jiegillet · 2019-10-12T05:45:35Z

I've added a property test. There are so many tests in the project, I'm not sure where I should have placed mine, let me know if I should move it.
The property is general enough to test any (Inject a, Interpret a).

By the way, while I was playing with it, I saw the property fail for Scientific (due to some rounding errors, I suppose that's to be expected) and Text (I think due to non-printable characters, probably because I use show in the test, let me know if there is a better way).

Gabriella439 · 2019-10-12T16:02:54Z

dhall/tests/Dhall/Test/QuickCheck.hs

@@ -432,6 +439,12 @@ normalizingAnExpressionDoesntChangeItsInferredType expression =
    filterOutEmbeds :: Typer a
    filterOutEmbeds _ = Const Sort -- This could be any ill-typed expression.

+injectThenInterpretIsIdentity :: (Inject a, Interpret a, Eq a) => a -> Property


If you add a Typeable constraint to this then you can have this generate not only the Property, but the description, too:

injectThenInterpretIsIdentity a = ( "Injecting then Interpreting is identity for " <> show (typeOf a) , property , QuickCheckTests 100 ) where property = monadicIO $ do …

Oooh, tasty :) (no pun intended)

Gabriella439 · 2019-10-12T16:03:19Z

dhall/src/Dhall.hs

@@ -1860,6 +1857,34 @@ instance Inject a => Inject (Data.HashSet.HashSet a) where

 instance (Inject a, Inject b) => Inject (a, b)

+{-| Inject a `Data.Map` to a @Prelude.Map.Type@
+
+>>> prettyExpr $ embed inject (Data.Map.fromList [(1 :: Integer, True )])


Usually for examples I try to use Natural instead of Integer (to encourage more people to use Natural)

jiegillet · 2019-10-13T12:42:53Z

Error in the automated tests:

C:\projects\dhall-haskell\dhall\tests\Dhall\Test\QuickCheck.hs:8:14:
    Unsupported extension: TypeApplications

Suggestions? :(

More:

Cabal-simple_Z6RU0evB_1.22.5.0_ghc-7.10.3.exe: The package dhall-1.26.1
    requires the following language extensions which are not supported by
    ghc-7.10.3: TypeApplications

Should I just not use TypeApplications?

sjakobi

👍 Very nice tests!

sjakobi · 2019-10-13T23:13:28Z

dhall/tests/Dhall/Test/QuickCheck.hs

@@ -56,6 +66,7 @@ import qualified Dhall.Map
 import qualified Dhall.Set
 import qualified Dhall.TypeCheck
 import qualified Generic.Random
+import qualified GHC.Natural as GHCNat


The docs for that module say:

Note: This is an internal GHC module with an API subject to change. It's recommended use the Numeric.Natural module to import the Natural type.

Suggested change

import qualified GHC.Natural as GHCNat

import qualified Numeric.Natural

sjakobi · 2019-10-13T23:24:07Z

dhall/tests/Dhall/Test/QuickCheck.hs

+    )
+  where
+    prop a = monadicIO $ do
+        a' <- run . input auto . Text.pack . show . prettyExpr . embed inject $ a


Why do you pretty-print and re-parse the embedded expressions?

I you'd simply extract the embedded Expr, we wouldn't need IO…

Oh man... I feel I'm doing so many obvious mistakes. Thank you!

No worries! :)

dhall/dhall.cabal

Co-Authored-By: Simon Jakobi <[email protected]>

jiegillet added 5 commits October 10, 2019 23:24

[#1392] Injecting Data.Map

8dc3177

injecting via Generic

e68548e

Explicit instance

34292ee

Merge branch 'master' into jie-map

89661a4

Merge github.com:dhall-lang/dhall-haskell into jie-map

ec45428

sjakobi reviewed Oct 10, 2019

View reviewed changes

dhall/src/Dhall.hs Outdated Show resolved Hide resolved

dhall/src/Dhall.hs Outdated Show resolved Hide resolved

dhall/src/Dhall.hs Outdated Show resolved Hide resolved

dhall/src/Dhall.hs Outdated Show resolved Hide resolved

jiegillet added 4 commits October 11, 2019 09:25

Cleanup code

c0ddb28

Merge branch 'master' of github.com:dhall-lang/dhall-haskell

4d3db17

Merge branch 'master' into jie-map

057ec54

Merge branch 'jie-map' of github.com:dhall-lang/dhall-haskell into ji…

ad1b530

…e-map

sjakobi reviewed Oct 11, 2019

View reviewed changes

jiegillet added 2 commits October 11, 2019 23:26

Minor code cleanup

c711b1c

Property testing for Inject/Interpret

1b9587f

Gabriella439 approved these changes Oct 12, 2019

View reviewed changes

Refactor test

3425a31

jiegillet added 2 commits October 13, 2019 21:43

Adding TypeApplications to .cabal

19905be

No more TypeApplications

56fc367

sjakobi reviewed Oct 13, 2019

View reviewed changes

jiegillet added 2 commits October 14, 2019 16:33

Simplified tests using extract

ddc5d06

Added test for Text amnf fixed Natural import

965a49c

sjakobi approved these changes Oct 14, 2019

View reviewed changes

sjakobi reviewed Oct 14, 2019

View reviewed changes

dhall/dhall.cabal Outdated Show resolved Hide resolved

dhall/dhall.cabal Outdated Show resolved Hide resolved

jiegillet and others added 2 commits October 15, 2019 07:31

Update dhall/dhall.cabal

5ecbd8f

Co-Authored-By: Simon Jakobi <[email protected]>

Update dhall/dhall.cabal

b9cbdc8

Co-Authored-By: Simon Jakobi <[email protected]>

jiegillet added the merge me label Oct 15, 2019

mergify bot added 2 commits October 15, 2019 10:01

Merge branch 'master' into jie-map

7d658e6

Merge branch 'master' into jie-map

9174be0

This was referenced Oct 15, 2019

Injecting Data.Map #1392

Closed

Marshalling HashMap #1394

Closed

mergify bot merged commit 1cd856a into master Oct 15, 2019

mergify bot deleted the jie-map branch October 15, 2019 11:28

	mapEntries = fmap recordPair . Data.Sequence.fromList . Data.Map.toList
	mapEntries = Data.Sequence.fromList . map recordPair . Data.Map.toList

	import qualified GHC.Natural as GHCNat
	import qualified Numeric.Natural

[#1392] Injecting Data.Map #1412

[#1392] Injecting Data.Map #1412

Uh oh!

Conversation

jiegillet commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sjakobi commented Oct 10, 2019

Uh oh!

sjakobi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiegillet commented Oct 11, 2019

Uh oh!

sjakobi commented Oct 11, 2019

Uh oh!

jiegillet commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiegillet commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjakobi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jiegillet commented Oct 10, 2019 •

edited

Loading

jiegillet commented Oct 12, 2019 •

edited

Loading

jiegillet commented Oct 13, 2019 •

edited

Loading