Re-export pack, unpack and Text from Data.Text #208

newhoggy · 2021-09-25T07:10:06Z

Re-export some Data.Text modules for the purpose of downstream libraries being able to write code compatible with the fake text module

Related issue: #207

sjakobi

Thanks for the PR, @newhoggy!

I'm pretty apprehensive about enhancing the code and API for the -f-text variant, but maybe I just need to better understand the motivation.

prettyprinter/prettyprinter.cabal

prettyprinter/src-text/Data/Text/Lazy/Builder.hs

newhoggy · 2021-09-28T07:31:30Z

I've adopted the recommendations in #207 (comment)

sjakobi

Unfortunately I have some concerns about the "future-proofness" of this approach.

prettyprinter/src/Prettyprinter/Util/TextCompat.hs

sjakobi · 2021-09-29T11:52:29Z

Please ignore the CI failure BTW. It's probably related to https://gitlab.haskell.org/haskell/ghcup-hs/-/issues/123.

sjakobi

Thanks!

Can you take a look at the failing CI jobs? Ignore the no-text job with GHC 7.10.3, which is broken, but should hopefully be fixed soon (#212).

prettyprinter/src/Prettyprinter/Util/Compat/Text.hs

sjakobi · 2021-11-02T23:25:09Z

CI on master should be fixed now. Please rebase!

newhoggy · 2021-11-09T08:40:02Z

The workflow for this PR is not running, but I've verified it builds independently here: input-output-hk#1

sjakobi

Looking good.

One (hopefully) last issue for me:

Since the new modules will be included in the PVP scheme, I think it would be best if we had explicit export lists there. Otherwise, it would be easy to change or remove one of the exposed functions without doing the necessary major version bump.

sjakobi · 2021-11-11T03:45:14Z

prettyprinter/src/Prettyprinter/Util/Compat/Text.hs

+-- Legitimate examples of packages that must have text as an optional dependency, include text (or
+-- bytetring).


Suggested change

-- Legitimate examples of packages that must have text as an optional dependency, include text (or

-- bytetring).

-- Legitimate examples of packages that must have text as an optional dependency, include text and

-- bytestring.

Same suggestion for the other new modules.

newhoggy · 2021-11-11T12:46:33Z

Added explicit export list and updated the docs "or" -> "and".

sjakobi

One wibble left.

Thank you! This was more work than expected.

prettyprinter/src/Prettyprinter/Util/Compat/Text.hs

…ies being able to write code compatible with the fake text module

newhoggy · 2021-11-16T08:01:14Z

I've pushed the requested updates.

newhoggy · 2021-11-26T02:03:44Z

Will we be able to merge this?

sjakobi · 2021-11-26T14:53:21Z

Ping, @quchen!

quchen

LGTM. The whole »emulating text« part is new to me so I did some digging; it makes quite a bit of sense now that Prettyprintert is becoming more and more standard.

quchen · 2021-12-03T13:18:09Z

prettyprinter/bench/LargeOutput.hs

+{-# LANGUAGE DeriveGeneric        #-}
+{-# LANGUAGE FlexibleInstances    #-}
+{-# LANGUAGE OverloadedStrings    #-}
+{-# LANGUAGE TypeSynonymInstances #-}


Is this change still necessary? I don’t see a change that would require those exts in this file.

I've removed FlexibleInstances and TypeSynonymInstances. Hopefully it still works. Please approve workflows.

quchen · 2021-12-03T13:21:04Z

prettyprinter/prettyprinter.cabal

+        , Prettyprinter.Util.Compat.Text
+        , Prettyprinter.Util.Compat.Text.IO
+        , Prettyprinter.Util.Compat.Text.Lazy
+        , Prettyprinter.Util.Compat.Text.Lazy.Builder


These will show up on Hackage, cluttering the package overview quite a bit – half of the whole package is already compatibility stuff. I don’t think there’s a way around this, or is there?

Optparse-Applicative is a very popular lib, so allowing Prettyprinter there would be a good thing. But I’m not a fan of cluttering prettyprinter’s public API just so some other package can retain eternal backwards compatibility – why can’t they depend on Text after all? Feels like a pretty strange design decision in anything later than 2010.

Perhaps one option is to move these to a separate package altogether?

newhoggy mentioned this pull request Sep 25, 2021

Need prettyprinter exports to convert between String and Text #207

Open

sjakobi reviewed Sep 27, 2021

View reviewed changes

prettyprinter/prettyprinter.cabal Outdated Show resolved Hide resolved

prettyprinter/src-text/Data/Text/Lazy/Builder.hs Show resolved Hide resolved

prettyprinter/src-text/Data/Text/Lazy/Builder.hs Show resolved Hide resolved

newhoggy force-pushed the re-export-data-text-modules branch from e834f12 to 4ad9ad5 Compare September 28, 2021 07:29

newhoggy requested a review from sjakobi September 28, 2021 07:32

newhoggy force-pushed the re-export-data-text-modules branch from 4ad9ad5 to cfa3bdb Compare September 28, 2021 07:35

newhoggy changed the title ~~Re-export some Data.Text modules~~ Re-export pack, unpack and Text from Data.Text Sep 28, 2021

sjakobi reviewed Sep 29, 2021

View reviewed changes

prettyprinter/src/Prettyprinter/Util/TextCompat.hs Outdated Show resolved Hide resolved

newhoggy force-pushed the re-export-data-text-modules branch from cfa3bdb to b47307b Compare November 2, 2021 08:57

sjakobi reviewed Nov 2, 2021

View reviewed changes

prettyprinter/src/Prettyprinter/Util/Compat/Text.hs Show resolved Hide resolved

Bodigrim mentioned this pull request Nov 3, 2021

Add pattern SString #214

Closed

newhoggy force-pushed the re-export-data-text-modules branch 3 times, most recently from 2e420c2 to 26d8dd4 Compare November 3, 2021 06:54

newhoggy requested a review from sjakobi November 3, 2021 06:54

newhoggy force-pushed the re-export-data-text-modules branch 3 times, most recently from d80f3d5 to e84d9fa Compare November 9, 2021 08:27

sjakobi reviewed Nov 11, 2021

View reviewed changes

sjakobi requested a review from quchen November 11, 2021 03:51

newhoggy force-pushed the re-export-data-text-modules branch 2 times, most recently from 4d3b61f to 04b5f99 Compare November 11, 2021 12:39

sjakobi approved these changes Nov 11, 2021

View reviewed changes

prettyprinter/src/Prettyprinter/Util/Compat/Text.hs Outdated Show resolved Hide resolved

Re-export some Data.Text modules for the purpose of downstream librar…

10c94e5

…ies being able to write code compatible with the fake text module

newhoggy force-pushed the re-export-data-text-modules branch from 04b5f99 to 10c94e5 Compare November 13, 2021 11:03

quchen approved these changes Dec 3, 2021

View reviewed changes

Remove surplus extensions

bb29b1e

newhoggy mentioned this pull request Feb 21, 2022

Upgrade optparse-applicative to use prettyprinter and add support for structured CLI help output IntersectMBO/cardano-node#3625

Open

BinderDavid mentioned this pull request Feb 23, 2024

prettyprinter-ansi-terminal is not compatible with -text option #250

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-export pack, unpack and Text from Data.Text #208

Re-export pack, unpack and Text from Data.Text #208

newhoggy commented Sep 25, 2021 •

edited by quchen

Loading

sjakobi left a comment

newhoggy commented Sep 28, 2021

sjakobi left a comment

sjakobi commented Sep 29, 2021

sjakobi left a comment

sjakobi commented Nov 2, 2021

newhoggy commented Nov 9, 2021

sjakobi left a comment

sjakobi Nov 11, 2021

newhoggy commented Nov 11, 2021

sjakobi left a comment

newhoggy commented Nov 16, 2021 •

edited

Loading

newhoggy commented Nov 26, 2021

sjakobi commented Nov 26, 2021

quchen left a comment

quchen Dec 3, 2021

newhoggy Jan 29, 2022

quchen Dec 3, 2021 •

edited

Loading

newhoggy Jan 29, 2022

		-- Legitimate examples of packages that must have text as an optional dependency, include text (or
		-- bytetring).

Re-export pack, unpack and Text from Data.Text #208

Are you sure you want to change the base?

Re-export pack, unpack and Text from Data.Text #208

Conversation

newhoggy commented Sep 25, 2021 • edited by quchen Loading

sjakobi left a comment

Choose a reason for hiding this comment

newhoggy commented Sep 28, 2021

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi commented Sep 29, 2021

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi commented Nov 2, 2021

newhoggy commented Nov 9, 2021

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi Nov 11, 2021

Choose a reason for hiding this comment

newhoggy commented Nov 11, 2021

sjakobi left a comment

Choose a reason for hiding this comment

newhoggy commented Nov 16, 2021 • edited Loading

newhoggy commented Nov 26, 2021

sjakobi commented Nov 26, 2021

quchen left a comment

Choose a reason for hiding this comment

quchen Dec 3, 2021

Choose a reason for hiding this comment

newhoggy Jan 29, 2022

Choose a reason for hiding this comment

quchen Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

newhoggy Jan 29, 2022

Choose a reason for hiding this comment

newhoggy commented Sep 25, 2021 •

edited by quchen

Loading

newhoggy commented Nov 16, 2021 •

edited

Loading

quchen Dec 3, 2021 •

edited

Loading