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

removed dependency on bytestring-conversion #8

Merged
merged 2 commits into from Jan 4, 2019

Conversation

esjmb
Copy link
Contributor

@esjmb esjmb commented Jan 4, 2019

Hi,
I'm suggesting the elimination of the dependency on bytestring-conversion, by replacing the use of Data.ByteString.Conversion.toByteString' with Data.Text.Encoding.encodeUtf8, and using encodeUtf8 directly in the ToHttpApiData Link and [Link] implementations, given that writeLinkHeader and writeLink return Text, and Data.ByteString.Conversion.toByteString' appears to resolve to a call to encodeUtf8 :: Text -> ByteString for Text. This allows the elimination of the ToByteString implementations for Link and [Link], which so far as I can tell are for internal use only.

The dependency on bytestring-conversion happens to be causing a build problem for stack --docker with LTS-13.1. This fix solves that. The dependency on text exists already.

results of stack test suggest no problems:

http-link-header-1.0.3.1: test (suite: tests)
            
Progress 1/2: http-link-header-1.0.3.1
Network.HTTP.Link.Parser
  linkHeader
    parses a single link
    parses empty attributes
    parses custom attributes
    parses backslash escaped attributes
    parses escaped attributes
    parses multiple attributes
    parses custom attributes named similarly to standard ones
    parses unquoted rel, rev attributes
    does not blow up on title*
    parses weird whitespace all over the place
Network.HTTP.Link.Writer
  writeLinkHeader
    writes a single link
    writes params with quote escaping
    writes multiple parameters
    writes custom params
    writes multiple links
Network.HTTP.Link
  writeLinkHeader → parseLinkHeader
    roundtrips successfully
      +++ OK, passed 100 tests.

Finished in 2.5769 seconds
16 examples, 0 failures
                                      
http-link-header-1.0.3.1: Test suite tests passed

thanks,

Stephen.

@valpackett
Copy link
Collaborator

I thought bytestring-conversion is the more efficient way, but whatever, less dependencies is good. Just clean up the commit :)

The dependency on bytestring-conversion happens to be causing a build problem for stack --docker with LTS-13.1

The actual problem should be fixed there too, did you report it?

@esjmb
Copy link
Contributor Author

esjmb commented Jan 4, 2019

I thought bytestring-conversion is the more efficient way, but whatever, less dependencies is good. Just clean up the commit :)

The dependency on bytestring-conversion happens to be causing a build problem for stack --docker with LTS-13.1

The actual problem should be fixed there too, did you report it?

So far as I can tell, the problem is with the fpco/stack-build for its 13.x. I've reported it there and I guess stack-build will be able to build byte string-conversion without difficulty in the near future.

There is a thread here: commercialhaskell/stack#4470

That issue led me to spot the unnecessary dependency for servant-auth (haskell-servant/servant-auth#136) , and then your http-link-header.

Quite possible that byte string-conversion is more efficient, I don't know, but from what I can tell, it seems that the servant-auth guys switched out dependency for bytestring-conversion to blaze a while ago.

@valpackett valpackett merged commit 0897d57 into sjshuck:master Jan 4, 2019
@valpackett
Copy link
Collaborator

Thanks!

@esjmb
Copy link
Contributor Author

esjmb commented Jan 4, 2019

Thanks!

and to you!

@esjmb esjmb deleted the remove_bc_dependency branch January 4, 2019 21:34
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.

None yet

3 participants