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

Add Lift ByteString instances #330

Closed
phadej opened this issue Nov 27, 2020 · 5 comments · Fixed by #392
Closed

Add Lift ByteString instances #330

phadej opened this issue Nov 27, 2020 · 5 comments · Fixed by #392
Milestone

Comments

@phadej
Copy link
Contributor

phadej commented Nov 27, 2020

See e.g. https://hackage.haskell.org/package/th-lift-instances-0.1.18/docs/src/Instances.TH.Lift.html#line-267

@Bodigrim
Copy link
Contributor

Bodigrim commented Nov 27, 2020

See also bennofs/th-lift-instances#23

Is this feature worth to incur an additional dependency on template-haskell?

@phadej
Copy link
Contributor Author

phadej commented Nov 28, 2020

Is this feature worth to incur an additional dependency on template-haskell?

Yes.

@Bodigrim
Copy link
Contributor

Thanks for the explanation, very convincing.

Given poor compatibility between releases of template-haskell package, I'm hesitant to agree blindly that defining instance Lift ByteString in bytestring brings more benefits than it causes headache. Even at this point it looks like one should use mkBytes with template-haskell >= 2.16, ByteString.unpack with template-haskell >= 2.8 and ByteString.Char8.unpack otherwise. This is a non-negligable amount of CPP.

Not that I have a set-in-stone attitude against it, I just do not use TemplateHaskell and do not know much about it.

@phadej
Copy link
Contributor Author

phadej commented Nov 29, 2020

Lifting of ByteString via TH is done very often. E.g. https://hackage.haskell.org/package/file-embed-0.0.13.0/docs/Data-FileEmbed.html#v:embedFile

embedFile :: FilePath -> Q Exp
embedFile fp =
#if MIN_VERSION_template_haskell(2,7,0)
    qAddDependentFile fp >>
#endif
  (runIO $ B.readFile fp) >>= bsToExp

bsToExp is lift.

Similar function is in th-lift-instances and who knows where because authors are not aware of other implementations or don't want depend on packages "just for one function". Having Lift ByteString in bytestring would allow packages converge to use the same implementation (and contributors improve single one).

Given poor compatibility between releases of template-haskell package, ...

That is not different from problems th-lift-instances maintainer(s) go through. Which in practice is probably @RyanGlScott propagating those changes downstream (after changing something in Template Haskell).

@Bodigrim
Copy link
Contributor

Bodigrim commented Dec 1, 2020

OK, makes sense.

Shimuuar added a commit to hexresearch/hschain-utxo that referenced this issue Dec 24, 2020
This required adding exception to get orphan instance. Hopefully it will be
added to bytestring proper

haskell/bytestring#330
Shimuuar added a commit to hexresearch/hschain-utxo that referenced this issue Dec 24, 2020
This required adding exception to get orphan instance. Hopefully it will be
added to bytestring proper

haskell/bytestring#330
@Bodigrim Bodigrim linked a pull request Jun 13, 2021 that will close this issue
@Bodigrim Bodigrim added this to the 0.11.2.0 milestone Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants