Instances for Record#171
Conversation
This commit is to be followed by several others, slowly building up the set of record instances in the prelude by way of RowToList.
Adding more and more type classes for records. We're getting somewhere!
SO MANY INSTANCES
As far as I can tell, there are now instances for all applicable typeclasses given for primitive records. I've even added tests, which typecheck (good news), though I don't think any of them actually run.
|
Hey looking nice, thanks for your work!! For things like So this might mean you'll have to implement ... or the other option is you parameterize P.S. Especially if you go with the former method, the you could just use Also, I'm curious why |
| , impliesRecordImpl | ||
| , conjRecordImpl | ||
| , disjRecordImpl | ||
| , notRecordImpl |
There was a problem hiding this comment.
Maybe we could drop Impl from all these kinds of names - usually that's for FFI things where there's a non-Impl version in scope too.
Perhaps the classes should be using Record in their name rather than Row too, since they are specialised to records in particular.
There was a problem hiding this comment.
Thinking about it, maybe all these should be defined in modules like Data.HeytingAlgebra.Record or something like that, that way they don't have to be exported from these "primary" modules, given that there's not really a need to use these functions outside of the record instances.
There was a problem hiding this comment.
Only problem here is that they all require things to have HeytingAlgebra constraints, which would cause a circular dependency :(
There was a problem hiding this comment.
In that case, how about putting all of this in Data.HeytingAlgebra.Internal, and then re-exporting the important bits from Data.HeytingAlgebra?
| @@ -0,0 +1,14 @@ | |||
| module Data.Internal.Record where | |||
There was a problem hiding this comment.
Data.Internal is a bit of a weird namespace - perhaps it could be Unsafe.Record like we have Unsafe.Coerce and Unsafe.Reference (unsafeRefEq).
There was a problem hiding this comment.
@LiamGoodacre mentioned potentially just merging Data.Record.Unsafe from purescript-record in here?
There was a problem hiding this comment.
which I guess also clears up the comment below, too :D Will sort this out (and remove focus from all the classes - thanks @MonoidMusician) at lunch 👍
| module Data.Internal.Record where | ||
|
|
||
| -- | *Really* unsafely get a value from a record. You really shouldn't be using | ||
| -- | this function unless you know what you're doing. |
There was a problem hiding this comment.
Perhaps we could refine these comments a bit 😄. The unsafeCoerce comment is pretty detailed actually: https://pursuit.purescript.org/packages/purescript-unsafe-coerce/3.0.0/docs/Unsafe.Coerce#v:unsafeCoerce
At least talking about the purpose / how to use them safely (such as it is) would be good.
|
Did some more work on this:
|
|
I'll merge this for now and we can make any further changes or fixes that are needed on the branch. Thanks @i-am-tom! |
|
You're brilliant, @garyb - thanks so much! |
Hullo!
This PR adds what I think/hope are reasonable implementations for all the standard type classes applicable to records. Please, please, please tear this apart, in case I've missed anything - being in the prelude, it is very tricky to test, and I'm worried I'll have missed something the type-checker wouldn't have caught.
However, if you can get the tests running, you probably don't need to worry. I, personally, couldn't, but I might be missing something obvious?
Thanks!