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

Use bytestring-builder instead of blaze-builder #55

Merged
merged 1 commit into from Feb 25, 2015

Conversation

Projects
None yet
4 participants
@RyanGlScott
Contributor

RyanGlScott commented Feb 23, 2015

Currently, fast-logger won't build correctly with blaze-builder-0.4 on GHC 7.6 (see here for example), since blaze-builder-0.4 doesn't export the Blaze.ByteString.Builder.Internal.Types module anymore. I think the simplest workaround is to remove the bytestring-builder dependency altogether on depend on bytestring-builder instead, which would reduce of the number of code paths due to CPP as a bonus.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 24, 2015

Collaborator

Note that this is a breaking change, so if merged should get a major version bump.

This seems reasonable to me. @manny-fp do you have any thoughts on this?

Collaborator

snoyberg commented Feb 24, 2015

Note that this is a breaking change, so if merged should get a major version bump.

This seems reasonable to me. @manny-fp do you have any thoughts on this?

@kazu-yamamoto

This comment has been minimized.

Show comment
Hide comment
@kazu-yamamoto

kazu-yamamoto Feb 24, 2015

Owner

I like this approach.

@snoyberg Do you think we do the same thing for the wai family, too?

Owner

kazu-yamamoto commented Feb 24, 2015

I like this approach.

@snoyberg Do you think we do the same thing for the wai family, too?

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 24, 2015

Collaborator

@manny-fp tried very hard to avoid breaking changes in wai, so it still works with blaze-builder. It's more important for wai, as every WAI application out there is potentially using blaze-builder already, and would require changes to work with such a change. However, I don't think the same arguments apply (or at least not as strongly) to fast-logger.

Collaborator

snoyberg commented Feb 24, 2015

@manny-fp tried very hard to avoid breaking changes in wai, so it still works with blaze-builder. It's more important for wai, as every WAI application out there is potentially using blaze-builder already, and would require changes to work with such a change. However, I don't think the same arguments apply (or at least not as strongly) to fast-logger.

@kazu-yamamoto

This comment has been minimized.

Show comment
Hide comment
@kazu-yamamoto

kazu-yamamoto Feb 24, 2015

Owner

OK. I understand and agree. Let's not change wai and let's merge this pull request.

Owner

kazu-yamamoto commented Feb 24, 2015

OK. I understand and agree. Let's not change wai and let's merge this pull request.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 24, 2015

Collaborator

@kazu-yamamoto Did you want me to merge this in? I typically hold off on making merges on fast-logger for you, but can do so if you'd like.

Collaborator

snoyberg commented Feb 24, 2015

@kazu-yamamoto Did you want me to merge this in? I typically hold off on making merges on fast-logger for you, but can do so if you'd like.

@borsboom

This comment has been minimized.

Show comment
Hide comment
@borsboom

borsboom Feb 24, 2015

@snoyberg I'd say if a breaking change is acceptable then this seems like the best way to go. Note I think this would still be compatible with code that uses blaze-builder >=0.4 (since it shares the same Builder datatype with bytestring-builder), but will break with blaze-builder <0.4.

That being said, it doesn't look like it would be very difficult to make this work with both versions. It's really close to what I did for warp, in fact.

borsboom commented Feb 24, 2015

@snoyberg I'd say if a breaking change is acceptable then this seems like the best way to go. Note I think this would still be compatible with code that uses blaze-builder >=0.4 (since it shares the same Builder datatype with bytestring-builder), but will break with blaze-builder <0.4.

That being said, it doesn't look like it would be very difficult to make this work with both versions. It's really close to what I did for warp, in fact.

@RyanGlScott

This comment has been minimized.

Show comment
Hide comment
@RyanGlScott

RyanGlScott Feb 24, 2015

Contributor

I went with this approach for the pull request simply because exposing different Builder data types depending on which version of bytestring was being used seemed a bit funny, and I'm not sure how you'd access the internal types with blaze-builder >= 0.4. I'd be okay with supporting the original scheme too if it can work (which you indicate is possible).

Contributor

RyanGlScott commented Feb 24, 2015

I went with this approach for the pull request simply because exposing different Builder data types depending on which version of bytestring was being used seemed a bit funny, and I'm not sure how you'd access the internal types with blaze-builder >= 0.4. I'd be okay with supporting the original scheme too if it can work (which you indicate is possible).

@snoyberg snoyberg merged commit 4003bd8 into kazu-yamamoto:master Feb 25, 2015

snoyberg added a commit that referenced this pull request Feb 25, 2015

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 25, 2015

Collaborator

@kazu-yamamoto I've merged in this pull request and bumped the version number on fast-logger appropriately. I haven't released to Hackage, but can do so if (1) you'd like me to, and (2) you add me as a maintainer on Hackage.

Collaborator

snoyberg commented Feb 25, 2015

@kazu-yamamoto I've merged in this pull request and bumped the version number on fast-logger appropriately. I haven't released to Hackage, but can do so if (1) you'd like me to, and (2) you add me as a maintainer on Hackage.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 25, 2015

Collaborator

Thanks @RyanGlScott for the PR!

Collaborator

snoyberg commented Feb 25, 2015

Thanks @RyanGlScott for the PR!

@kazu-yamamoto

This comment has been minimized.

Show comment
Hide comment
@kazu-yamamoto

kazu-yamamoto Feb 26, 2015

Owner

@snoyberg I added you to the maintainer group. Please upload fast-logger by yourself because I'm a little bit busy now.

Owner

kazu-yamamoto commented Feb 26, 2015

@snoyberg I added you to the maintainer group. Please upload fast-logger by yourself because I'm a little bit busy now.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 26, 2015

Collaborator

Cool, released :)

Collaborator

snoyberg commented Feb 26, 2015

Cool, released :)

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