Skip to content

Use Data.Text.Lazy.Builder in ANSI rendering#29

Merged
jgm merged 2 commits intojgm:masterfrom
silby:builder
Mar 14, 2024
Merged

Use Data.Text.Lazy.Builder in ANSI rendering#29
jgm merged 2 commits intojgm:masterfrom
silby:builder

Conversation

@silby
Copy link
Contributor

@silby silby commented Mar 13, 2024

We have to smush a bunch of strings together in a fold with auxiliary data and hit a quadratic time blowup. This change means we return a lazy Text from renderANSI instead of whatever the contents of Doc a were but in the relevant contexts I don't think anyone will mind.

We have to smush a bunch of strings together in a fold with auxiliary
data and hit a quadratic time blowup. This change means we return a lazy
Text from renderANSI instead of whatever the contents of `Doc a` were
but in the relevant contexts I don't think anyone will mind.
@silby silby mentioned this pull request Mar 13, 2024
@jgm
Copy link
Owner

jgm commented Mar 13, 2024

This would be a breaking API change, so we'd need to bump the version accordingly. I think it's probably okay, though. In pandoc, anyway, a lazy text would be just fine.

A more conservative change might be to have a build and an unbuild. This could use text's builder for text values, bytestring's builder for bytestring values. I don't know if it's worth the extra complexity, though.

@silby
Copy link
Contributor Author

silby commented Mar 13, 2024

renderANSI hasn't gone into a tagged version yet, right? render/renderPlain still returns a. But I see what you're saying about adding unbuild, that sounds like the Right Thing To Do.

@jgm
Copy link
Owner

jgm commented Mar 13, 2024

Oh, I see! I didn't realize render and renderPlain hadn't changed.
Well, then I think this would be fine, unless there's some good reason for doing the other thing.

@jgm
Copy link
Owner

jgm commented Mar 13, 2024

I guess it's still an API change, because of the new method on HasChars, but that's unlikely to break anything.

This means that any previously-complete definitions of HasChars from
before adding build to the typeclass will not be broken.
@silby
Copy link
Contributor Author

silby commented Mar 13, 2024

Added a commit adding a default build implementation using foldrChar.

@jgm
Copy link
Owner

jgm commented Mar 14, 2024

Can you make sure your commit message explicitly notes the API change to HasChars? So that I won't forget it in the changelog (and will remember to bump version according to the policy).

@jgm jgm merged commit 625482f into jgm:master Mar 14, 2024
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.

2 participants