Add field to Result which allows storage of arbitrary data #381
Conversation
This feature is need for tasty-bench and tasty-papi which currently sneak data in `resultDescription' and then treat this field specially. But such feature seems generally useful.
core/Test/Tasty/Core.hs
Outdated
| -- | Lookup values of given type o | ||
| -- | ||
| -- @since NEXTVERSION | ||
| lookupExtraData :: Typeable a => Result -> Maybe a | ||
| lookupExtraData = asum . map fromDynamic . resultExtraData | ||
|
|
||
| -- | Attach value of arbitrary type to result of execution | ||
| -- | ||
| -- @since NEXTVERSION | ||
| attachExtraData :: Typeable a => a -> Result -> Result | ||
| attachExtraData a r = r { resultExtraData = toDyn a : resultExtraData r } |
There was a problem hiding this comment.
I don't like that you can attach multiple values of the same type here but only get one of them back. Possible suggestions:
- Use a map from
TypeReptoDynamicso we can only have oneDynamicper type - Have
lookupExtraDatajust return a list ofmapMaybe fromDynamic.
There was a problem hiding this comment.
Is it even plausible to have multiple extra data to the same Result? I'd imagine that each result is produced by a single test provider, so resultExtraData :: Maybe Dynamic should suffice.
There was a problem hiding this comment.
@ocharles re [Dynamic] vs Map TypeRep Dynamic. Both has ugly side. Here I treat list as append only map which retain history of updates and old versions are kept around for no reason. Map variant allows adding (manually) elements with non-matching TypeReps. Either way I don't feel strongly about representation. Returning list of results seems inconvenient to use.
@Bodigrim I prefer ask opposite question. Is there good reason to prohibit attaching multiple pieces of data? I don't see any. Maybe Dynamic will restrict possible uses without gaining anything out of it. It is possible to invent possible uses: two different options to generate report where each generator expects data in different format.
There was a problem hiding this comment.
Dynamic is big enough already and includes any type, including lists of other Dynamic. In a certain sense even Maybe Dynamic is one element too big.
I think I'd prefer to be more explicit and unwrap Dynamic, making Result a GADT hiding type of resultExtraData :: Typeable a => a. But I'd like to hear more opinions before deciding on this.
There was a problem hiding this comment.
What is benefit of single Dynamic? To it looks like a strictly worse alternative. If one has two distinct pieces of data A and B sure one may attach tuple (A,B) but then there's no way to look up only B. This proposal brings features from dynamically typed languages: ability to add arbitrary fields to objects. Here keyed by haskell types instead of strings. Having single fields seems needlessly limiting to me.
GADT is especially problematic. It's liable to cause problem with type checking, record updates, record dot.
There was a problem hiding this comment.
It's just a coincidence that Result does not declare instances other than Show, otherwise Dynamic would not work. I do not want to prohibit defining potential instance Eq Result and such in future.
So I think data SomeExtraInfo = forall a. (Typeable a, Ord a, Show a, NFData a) => a or similar is a better choice.
There was a problem hiding this comment.
Yes. That's quite reasonable. I've amended PR and added Read too. There're two complications:
- There's no sensible implementation for
Ord. We'll need to compare different types and only way to do so is viaTypeRepwhich use some sort of hash internally so I don't think such order would be stable wrt to package/compiler versions. I'm not sure it's good idea to add such instances - Adding
NFDatawill require adding dependency ondeepseq
This change is to carry mode type class dictionaries and to allow to define more possible instances for Result.
This is proposal to add field to
Resultwhich allows attaching arbitrary data to it. Change is relatively small so I think it's easier to create PR and discuss concrete code than try to describe and everyone would understand it differently.This change mostly benefits
tasty-bench&tasty-papiwhich currently sneak data inresultDescriptionfield. But to me it seems generally useful.Storage is implemented as list of
Dynamic. I expect that number of elements would be small (0 or 1) so linear scan is acceptable.