Add Lift instances for Text. #232
Conversation
|
I'd prefer to delay this to the next release; as 1.2.3.1 is the conservative one supposed to ship as point/fix rls w/ GHC 8.6.1 (and possibly GHC 8.4.4) and I'd like to avoid last minute changes (especially API additions) w/o having had the time to think them through |
|
Yeah, no rush on this. This has just annoyed me one too many times and I figured I'd send a patch. |
|
1.2.3.1 has been released – perhaps its time to revisit this PR. |
|
I'm very sympathetic to his PR; @quasicomputational could you come up with a couple simple test-cases that execute the new codepaths? |
aae5f26 to
b355429
Compare
|
Good call on the tests. I could have sworn I checked it was working with GHCi, but, having written the tests, it seems like the Along the way, I got bitten by a cabal-install bug, haskell/cabal#5623. That's why the TH-requiring tests are in their own little package. |
6c5d11a to
60e5524
Compare
| @@ -1,4 +1,4 @@ | |||
| {-# LANGUAGE BangPatterns, CPP, MagicHash, Rank2Types, UnboxedTuples #-} | |||
There was a problem hiding this comment.
Requiring full TH to build text is not acceptable; however, TemplateHaskellQuotes would be acceptable
There was a problem hiding this comment.
This is awkward. TemplateHaskellQuotes is only since 8.0, and I'm drawing a blank on how to write this without quote syntax: mkName and lookupValueName operate at the use site, which is unsuitable (what if users don't import Data.Text? What if it's been renamed?).
Might have to put this on ice until 7.10 and lower are out of the support window.
There was a problem hiding this comment.
Well, if you can make it work for GHC >= 8 with THQ, we can conditionally enable old-style TH for GHC < 8 . We just need to make sure that recentish GHCs don't require to have a bytecode interpreter in order to build the core library text
These have similar trade-offs to the existing `Data` instances: preserving abstraction at the cost of efficiency. Due to haskell/cabal#5623, the tests exercising this feature have to live in their own package.
|
I'm seeing a proptest failure in |
| deepseq >= 1.1.0.0, | ||
| ghc-prim >= 0.2 | ||
| ghc-prim >= 0.2, | ||
| template-haskell |
There was a problem hiding this comment.
Hi, just curious, was any thought put into adding this extra dependency? Now, packages which use text will now need to build/link template-haskell. Is this not that a big enough issue for most people?
There was a problem hiding this comment.
Note that template-haskell is considered a non-reinstallable package; i.e. it's only built once when GHC itself is build. Also note this doesn't require a GHC with TH engine either (i.e. text still works perfectly well with a GHC that cannot run TH). But why do you consider the need to link against template-haskell a problem?
There was a problem hiding this comment.
If a Haskell executable statically links their dependencies, wouldn't it become non-trivially bigger having to statically link template-haskell, even if they aren't using the Lift instance for Text?
These use the existing
Datainstances, and have similar trade-offs.I've also bumped the version number appropriately, but I don't know if that'd be better done later (e.g., by the release manager near the time of release).