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

Link header #1687

Merged
merged 8 commits into from Nov 5, 2018

Conversation

Projects
None yet
4 participants
@ioganegambaputifonguser
Contributor

ioganegambaputifonguser commented Mar 6, 2018

Enhancemet #760 Add simple link header with URI-reference only for now. Add test with such header.

Fedor Shiriaev
@@ -5,13 +5,15 @@ import cats.effect._
import cats.implicits._
import java.nio.ByteBuffer
import java.nio.charset.StandardCharsets

This comment has been minimized.

@aeons

aeons Mar 6, 2018

Member

Imports should be sorted alphabetically without any blank lines.

This comment has been minimized.

@ioganegambaputifonguser

ioganegambaputifonguser Mar 6, 2018

Contributor

got it
btw do you know why build fails with my Link.scala with the format issues i guess?
locally it passed with the test:scalafmt

This comment has been minimized.

@aeons

aeons Mar 6, 2018

Member

test:scalafmt only formats test sources. You need to also run scalafmt to format main sources.

You can run sbt ci to do the same as what ci runs. It takes a while though.

@aeons

This comment has been minimized.

Member

aeons commented Mar 6, 2018

This is a good start.

I don't know much about the Link header, but from skimming the RFC linked to in the issue, it seems the parser is a bit too simplistic, and will fail on most of the examples from the RFC.

@ioganegambaputifonguser

This comment has been minimized.

Contributor

ioganegambaputifonguser commented Mar 6, 2018

sure, I have to put WIP status
Also I am going to add a few link params within the current PR, using the RFC what you mentioned

Link           = "Link" ":" #link-value
  link-value     = "<" URI-Reference ">" *( ";" link-param )
  link-param     = ( ( "rel" "=" relation-types )
                 | ( "anchor" "=" <"> URI-Reference <"> )
                 | ( "rev" "=" relation-types )
                 | ( "hreflang" "=" Language-Tag )
                 | ( "media" "=" ( MediaDesc | ( <"> MediaDesc <"> ) ) )
                 | ( "title" "=" quoted-string )
                 | ( "title*" "=" ext-value )
                 | ( "type" "=" ( media-type | quoted-mt ) )
                 | ( link-extension ) )

@ioganegambaputifonguser ioganegambaputifonguser changed the title from Link header to WIP: Link header Mar 6, 2018

@ioganegambaputifonguser ioganegambaputifonguser changed the title from WIP: Link header to Link header Mar 12, 2018

Fedor Shiriaev
val buf = Await.result(runRequest(Seq(req), service), 5.seconds)
val (_, hdrs, _) = ResponseParser.apply(buf)
hdrs.find(_.name == Link.name) must beSome(linkHeader)
}

This comment has been minimized.

@rossabaker

rossabaker Mar 22, 2018

Member

This header shouldn't really need to be tested in blaze.

If you can add an Arbitrary[Link] (I'll help if you can't), then you can follow a model more like the other header specs. It'll be a simple test that covers more.

@rossabaker rossabaker added enhancement and removed enhancement labels Mar 26, 2018

Fedor Shiriaev and others added some commits Apr 4, 2018

@ioganegambaputifonguser

This comment has been minimized.

Contributor

ioganegambaputifonguser commented Apr 4, 2018

@rossabaker somehow I missed a notification that there are some updates. So I am here and ready to finish with it. I added Arbitrary[Link] with the uri only support. Also I removed test from the braze-server. But some of the tests fails from the checkAll(name="Link", headerLaws(Link)) :(. Could you please support a little with it? Thanks!

@rossabaker

This comment has been minimized.

Member

rossabaker commented Apr 11, 2018

Sorry, now it was my turn to miss a notification.

This failure is not obvious to me, but I strongly suspect it's related to the problem described in #1651. If a URI doesn't round trip through the parser as the same structure, then the header containing them will also have problems. I think if we fix that, this will also begin to work.

Yet another reason we need to fix our Uri type. :(

@rossabaker

This comment has been minimized.

Member

rossabaker commented Sep 6, 2018

Wow, this one fell off the radar. I'm so sorry.

The proper fix depends on fixing our Uri, but we've been busy with fs2 and cats-effect. Is this something that we could mark the tests pending and get the header?

@rossabaker rossabaker added this to the 0.19.0-RC1 milestone Sep 30, 2018

rossabaker added some commits Oct 2, 2018

@rossabaker rossabaker merged commit 47f1ca8 into http4s:master Nov 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment