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

Adds Builder Show instance #296

Merged
merged 4 commits into from
Oct 5, 2020
Merged

Conversation

elikoga
Copy link
Contributor

@elikoga elikoga commented Oct 2, 2020

As mentioned in #47 Builder and friends should have Show instances.

I have added a Show instance inspired by the Show instance of Set. If it should be styled differently just mention it.

If I should add any more instances just mention it.

@Bodigrim
Copy link
Contributor

Bodigrim commented Oct 3, 2020

I agree with the proposed approach in general. However lazyByteString "12345" requires {-# LANGUAGE OverloadedStrings #-} enabled. How do you feel about string8 instead of lazyByteString?

@fumieval
Copy link
Contributor

fumieval commented Oct 3, 2020

Why not just "12345"? Other Show ByteString instances require OverloadedStrings as well.

@Bodigrim
Copy link
Contributor

Bodigrim commented Oct 3, 2020

@fumieval even better, I forgot that there is instance IsString Builder. Could you possibly suggest which other data types from Data.ByteString.Builder.* may benefit from having Show instance?

@elikoga
Copy link
Contributor Author

elikoga commented Oct 3, 2020

Done

@fumieval
Copy link
Contributor

fumieval commented Oct 3, 2020

Could you possibly suggest which other data types from Data.ByteString.Builder.* may benefit from having Show instance

There are Next, FixedPrim and BoundedPrim, but none of them has a reasonable Show instance.

@elikoga
Copy link
Contributor Author

elikoga commented Oct 3, 2020

What would be reasonable Show instances for Next, FixedPrim and BoundedPrim.

I can't think of any for FixedPrim and BoundedPrim and Next doesn't seem easy either.

@elikoga
Copy link
Contributor Author

elikoga commented Oct 3, 2020

One idea for a FixedPrim a and BoundedPrim a instance would be to only show their Fixed/Bounded size while not showing the IO action dependent on a.

@fumieval
Copy link
Contributor

fumieval commented Oct 3, 2020

@elikoga no, we should not add Show instances to them.

Data/ByteString/Builder.hs Outdated Show resolved Hide resolved
@Bodigrim Bodigrim added this to the 0.11.1.0 milestone Oct 5, 2020
@Bodigrim Bodigrim linked an issue Oct 5, 2020 that may be closed by this pull request
@Bodigrim Bodigrim merged commit 7851d7a into haskell:master Oct 5, 2020
@Bodigrim
Copy link
Contributor

Bodigrim commented Oct 5, 2020

Thanks, @elikoga!

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

Successfully merging this pull request may close these issues.

Builder and friends should have Show instances
3 participants