Skip to content

fixes #955 adding client recipe to cookbook#962

Merged
vkostyukov merged 1 commit into
finagle:masterfrom
erikwj:finagle-client
Jul 14, 2018
Merged

fixes #955 adding client recipe to cookbook#962
vkostyukov merged 1 commit into
finagle:masterfrom
erikwj:finagle-client

Conversation

@erikwj
Copy link
Copy Markdown
Contributor

@erikwj erikwj commented Jul 13, 2018

No description provided.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 13, 2018

Codecov Report

Merging #962 into master will decrease coverage by 0.6%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #962      +/-   ##
=========================================
- Coverage   83.31%   82.7%   -0.61%     
=========================================
  Files          50      50              
  Lines         827     827              
  Branches       49      48       -1     
=========================================
- Hits          689     684       -5     
- Misses        138     143       +5
Impacted Files Coverage Δ
core/src/main/scala/io/finch/Input.scala 90.38% <0%> (-9.62%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 856e673...02ab03c. Read the comment docs.

Comment thread docs/src/main/tut/cookbook.md Outdated

val host = "api.nasa.gov"
val client: Service[Request, Response] = Http.client.withTls(host).newService(s"$host:443")
val request = Request(Method.Get, "/planetary/apod?api_key=NNKOjkoul8n1CH18TWA9gwngW1s1SmjESPjNoUFo")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I like using NASA for the example, I'm a little concerned about having this api key here for a couple reasons. Is this key always going to be valid? Whose is it? Are there rate limits applied to it? We may want to consider using something a little more stable.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use DEMO_KEY instead? This https://api.nasa.gov/planetary/apod?api_key=DEMO_KEY works for me. They have some limits about the DEMO_KEY usage but I think we should be good: https://api.nasa.gov/api.html#demo_key-rate-limits

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to using the demo key.

Comment thread docs/src/main/tut/cookbook.md Outdated
val response: Future[Response] = client(request)

Await.result(response.onSuccess { rep: Response =>
//println("GET success: " + rep.contentString)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to remove this commented out line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpless Agree on all above will update

@rpless
Copy link
Copy Markdown
Collaborator

rpless commented Jul 13, 2018

@erikwj Thanks for putting this together! I left a couple comments. Curious to hear your thoughts.

@vkostyukov
Copy link
Copy Markdown
Collaborator

Thanks, @erikwj! This LGTM (after @rpless's concern about the API key is addressed).

val corsService: Service[Request, Response] = new Cors.HttpFilter(policy).andThen(service)
```

### Creating a Finagle Client
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a TOC item for this? I don't think we generate automatically.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Copy link
Copy Markdown
Collaborator

@vkostyukov vkostyukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, @erikwj! This looks good to me.

@vkostyukov vkostyukov merged commit 2f69735 into finagle:master Jul 14, 2018
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.

4 participants