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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 2 commits into from

Conversation

newhoggy
Copy link

@newhoggy newhoggy commented Sep 25, 2021 •
edited by quchen
Loading

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

@newhoggy newhoggy mentioned this pull request Sep 25, 2021
Open
sjakobi
sjakobi reviewed Sep 27, 2021
View reviewed changes
Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Copy link
Author

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

@newhoggy newhoggy changed the title Re-export some Data.Text modules Re-export pack, unpack and Text from Data.Text Sep 28, 2021
sjakobi
sjakobi reviewed Sep 29, 2021
View reviewed changes
Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

prettyprinter/src/Prettyprinter/Util/TextCompat.hs Outdated Show resolved Hide resolved
@sjakobi
Copy link
Collaborator

sjakobi commented Sep 29, 2021

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

sjakobi
sjakobi reviewed Nov 2, 2021
View reviewed changes
Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@sjakobi
Copy link
Collaborator

sjakobi commented Nov 2, 2021

CI on master should be fixed now. Please rebase!

@Bodigrim Bodigrim mentioned this pull request Nov 3, 2021
Closed
@newhoggy 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 newhoggy requested a review from sjakobi November 3, 2021 06:54
@newhoggy newhoggy force-pushed the re-export-data-text-modules branch 3 times, most recently from d80f3d5 to e84d9fa Compare November 9, 2021 08:27
@newhoggy
Copy link
Author

newhoggy commented Nov 9, 2021

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

sjakobi
sjakobi reviewed Nov 11, 2021
View reviewed changes
Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

prettyprinter/src/Prettyprinter/Util/Compat/Text.hs Outdated
Comment on lines 4 to 5
-- Legitimate examples of packages that must have text as an optional dependency, include text (or
-- bytetring).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sjakobi sjakobi requested a review from quchen November 11, 2021 03:51
@newhoggy newhoggy force-pushed the re-export-data-text-modules branch 2 times, most recently from 4d3b61f to 04b5f99 Compare November 11, 2021 12:39
@newhoggy
Copy link
Author

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

sjakobi
sjakobi approved these changes Nov 11, 2021
View reviewed changes
Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One wibble left.

Thank you! This was more work than expected.

prettyprinter/src/Prettyprinter/Util/Compat/Text.hs Outdated Show resolved Hide resolved
@newhoggy
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
Copy link
Author

newhoggy commented Nov 16, 2021 •
edited
Loading

I've pushed the requested updates.

@newhoggy
Copy link
Author

Will we be able to merge this?

@sjakobi
Copy link
Collaborator

sjakobi commented Nov 26, 2021

Ping, @quchen!

quchen
quchen approved these changes Dec 3, 2021
View reviewed changes
Copy link
Owner

@quchen quchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

prettyprinter/bench/LargeOutput.hs Outdated
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE TypeSynonymInstances #-}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

prettyprinter/prettyprinter.cabal
, Prettyprinter.Util.Compat.Text
, Prettyprinter.Util.Compat.Text.IO
, Prettyprinter.Util.Compat.Text.Lazy
, Prettyprinter.Util.Compat.Text.Lazy.Builder
Copy link
Owner

@quchen quchen Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@newhoggy newhoggy mentioned this pull request Feb 21, 2022
Open
@BinderDavid BinderDavid mentioned this pull request Feb 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quchen quchen quchen approved these changes

@sjakobi sjakobi sjakobi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@newhoggy @sjakobi @quchen