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

Make text an optional dependency #202

Merged
merged 4 commits into from
Aug 19, 2021
Merged

Make text an optional dependency #202

merged 4 commits into from
Aug 19, 2021

Conversation

Bodigrim
Copy link
Contributor

I should have raised an issue first, but it is easier to demonstate what I'm proposing with a PR. I'm fine to have it closed/rejected, if we find it unpalatable.

I very much understand that it's a core value of prettyprinter to use Text and not String. However, there are cases when you cannot afford it: namely, when you are a text (or bytestring) developer ;) or work with a very bleeding-edge GHC. If in future ansi-wl-pprint (already deprecated) is to extinct and be replaced by prettyprinter in dependencies of optparse-applicative, tasty and test-framework, developers of low-level packages would have to deal with pretty nasty build dependencies. E. g., to run bytestring tests you'd have to rebuild text, then prettyprinter, then optparse-applicative, then tasty and all its providers. This is extremely disheartening, as I can tell from earlier experience.

I was interested to check how difficult is to make prettyprinter fallback to String, if text is not available. It seems that just couple of lines and a Cabal flag would suffice. How do you feel about this?

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.

Nice hack! 😈 I'm curious what @quchen will say! ;)

I'd like to see:

  • Brief explanatory module headers for the contents of src-text.
  • CI with -text. I think this can be achieved with a haskell-ci constraint set.

prettyprinter/prettyprinter.cabal Outdated Show resolved Hide resolved
@sjakobi
Copy link
Collaborator

sjakobi commented Jul 25, 2021

Also, I hope that there are no expectations that prettyprinter maintainers will work on performance tuning the -text implementation.

@Bodigrim
Copy link
Contributor Author

I'm happy to supply CI config and additional comments and whatever :) if @quchen approves this approach.

There is also an option to separate src-text into an internal library, so that one can run tests and benchmarks in -text configuration. I've implemented it first, but reverted back, because it is a tiny bit more intrusive.

@quchen
Copy link
Owner

quchen commented Aug 2, 2021

Wow, this is some hack. If it helps adoption, I’m in favor. I think you should add documentation for the very much non-intuitive new modules, a simple paste of the ticket’s description as Haddock should suffice. It’s also important that this does not show up the official Haddock documentation or in standard builds, but that shouldn’t be the case by default anyway, right?

@Bodigrim
Copy link
Contributor Author

Bodigrim commented Aug 2, 2021

@quchen @sjakobi I've added more comments + CI build.

.github/workflows/no-text.yml Outdated Show resolved Hide resolved
.github/workflows/no-text.yml Show resolved Hide resolved
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.

Looks pretty good! A few more wibbles…

prettyprinter/prettyprinter.cabal Outdated Show resolved Hide resolved
prettyprinter/prettyprinter.cabal Outdated Show resolved Hide resolved
prettyprinter/test/Testsuite/Main.hs Outdated Show resolved Hide resolved
.github/workflows/no-text.yml Outdated Show resolved Hide resolved
@sjakobi
Copy link
Collaborator

sjakobi commented Aug 12, 2021

The previous CI run had timed out for some reason. I've restarted it now.

@Bodigrim
Copy link
Contributor Author

@sjakobi There appears to be a subtle issue with Stack support of internal libraries: while stack build && stack repl works, rm -rf .stack-work && stack repl does not. Unfortunately, from what I can gather, this seems to be the same kind of issues that hinder Backpack support in Stack and unlikely to be resolved any time soon. So I reverted a commit, covering test support (let it remain in git history, if someone in future will be interested to give it another go).

Otherwise this is ready for a final review, I believe.

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.

@Bodigrim Ah, that's unfortunate.

@quchen Let's merge this PR by rebasing or with a merge commit to preserve the commit history.

@sjakobi sjakobi merged commit 205b338 into quchen:master Aug 19, 2021
@sjakobi
Copy link
Collaborator

sjakobi commented Aug 19, 2021

Thank you, @Bodigrim! :) I'll try to start working on a release this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants