Missing Test and Docs #132

AdrianRaFo · 2017-05-15T15:31:40Z

From #131 , this PR covers:

activity: Docs missing return value
auth: Missing Docs
gist : Missing description integration test
gitdata: Missing Docs, Missing integration test
issue: Doc missing return value in comments
pullrequest: Docs misssing return value, Missing unit test
repo: Missing unit test
user: Missing Docs

Create empty docs Fix changelog Add description in integration test

BenFradet

First pass on the doc 👍

BenFradet · 2017-05-15T15:57:49Z

docs/src/main/tut/auth.md

+Github4s supports the [Authentication API](https://developer.github.com/v3/oauth_authorizations/). As a result,
+with github4s, you can:
+
+- [New authentication](#new-authentication)


Create a new authorization token would be better I think

BenFradet · 2017-05-15T15:58:10Z

docs/src/main/tut/auth.md

+with github4s, you can:
+
+- [New authentication](#new-authentication)
+- [Authorize url](#authorize-url)


Authorize a URL

BenFradet · 2017-05-15T15:58:20Z

docs/src/main/tut/auth.md

+
+- [New authentication](#new-authentication)
+- [Authorize url](#authorize-url)
+- [Get access token](#get-access-token)


Get an access token

BenFradet · 2017-05-15T15:58:44Z

docs/src/main/tut/auth.md

+
+## Authentication
+
+### New authentication


Create a new authorization token

BenFradet · 2017-05-15T16:01:34Z

docs/src/main/tut/auth.md

+
+
+ Call to request a new authorization given a basic authentication, the returned object Authorization includes an
+ access token


newAuth can be used to request a new auth token given basic authentication, the returned [Authorization][auth-scala] includes the created token. It takes as arguments:

BenFradet · 2017-05-15T16:33:59Z

docs/src/main/tut/user.md

+  def getAuth: GHIO[GHResponse[User]] = O.getAuthUser(accessToken)
+
+The `result` on the right is the corresponding [List[User]][user-scala].


BenFradet · 2017-05-15T16:34:06Z

docs/src/main/tut/user.md

+The `result` on the right is the corresponding [List[User]][user-scala].
+
+See [the API doc](https://developer.github.com/v3/activity/notifications/#set-a-thread-subscription) for full reference.


BenFradet · 2017-05-15T16:34:11Z

docs/src/main/tut/user.md

+    */
+  def getUsers(since: Int, pagination: Option[Pagination] = None): GHIO[GHResponse[List[User]]] =
+    O.getUsers(since, pagination, accessToken)


BenFradet · 2017-05-15T16:34:17Z

docs/src/main/tut/user.md

+The `result` on the right is the corresponding [List[User]][user-scala].
+
+See [the API doc](https://developer.github.com/v3/activity/notifications/#set-a-thread-subscription) for full reference.


BenFradet · 2017-05-15T16:34:27Z

docs/src/main/tut/user.md

+See [the API doc](https://developer.github.com/v3/activity/notifications/#set-a-thread-subscription) for full reference.
+
+As you can see, a few features of the activity endpoint are missing.


AdrianRaFo · 2017-05-16T08:59:40Z

Thanks for the feedback. A big part of the changes requested are from the wip

BenFradet · 2017-05-16T09:00:50Z

@AdrianRaFo I thought you still wanted a review since you requested reviewers, my bad.

set default pagination

juanpedromoreno · 2017-05-16T10:20:19Z

docs/src/main/tut/auth.md

+
+```
+
+They also make use of `cats.Id` but any type container implementing `MonadError[M, Throwable]` will


We could remove this sentence too

juanpedromoreno · 2017-05-16T10:20:59Z

docs/src/main/tut/auth.md

+# Authentication API
+
+Github4s supports the [Authentication API](https://developer.github.com/v3/oauth_authorizations/). As a result,
+with github4s, you can:


-with github4s +with Github4s

juanpedromoreno · 2017-05-16T10:22:49Z

docs/src/main/tut/auth.md

+
+### Create a new authorization token
+
+Used to request a new auth token given basic authentication


. at the end of the sentence.

juanpedromoreno · 2017-05-16T10:23:34Z

docs/src/main/tut/auth.md

+
+Used to request a new auth token given basic authentication
+
+You can create a tree using `newAuth`, it takes as arguments:


You can create a tree?

delete default pagination

BenFradet

Made another pass on the last commit 👍 .

I'm also wondering about the change to the default pagination?

BenFradet · 2017-05-16T10:49:36Z

docs/src/main/tut/auth.md


 ```scala
-val newAuth = Github(accessToken).auth.newAuth(
+val newAuth = Github(None).auth.newAuth(


Is this for indicating that no token is required? If so it might be nice to document it.

It's None because you don't have any accessToken yet, anyway i'm going to document it

BenFradet · 2017-05-16T10:50:17Z

docs/src/main/tut/auth.md

@@ -63,16 +60,16 @@ See [the API doc](https://developer.github.com/v3/oauth_authorizations/#create-a

 ### Authorize a url

-Generates the authorize url with a random state, both are returned within Authorize object
+Generates the authorize url with a random state, both are returned within Authorize object.


within an Authorize object.

BenFradet · 2017-05-16T10:53:51Z

docs/src/main/tut/issue.md

 ## Issues

 ### Create an issue

 You can create an issue using `createIssue`, it takes as arguments:

- the repository coordinates (owner and name of the repository)
- the content of the issue (title and body)
+- the repository coordinates (owner and name of the repository).


this is owner and name in the other doc (true for the others in issue.md)

codecov-io · 2017-05-16T11:07:33Z

Codecov Report

Merging #132 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #132   +/-   ##
=====================================
  Coverage      88%    88%           
=====================================
  Files          36     36           
  Lines         550    550           
  Branches        2      2           
=====================================
  Hits          484    484           
  Misses         66     66

Impacted Files	Coverage Δ
...b4s/shared/src/main/scala/github4s/api/Users.scala	`100% <ø> (ø)`	⬆️
...src/main/scala/github4s/free/algebra/UserOps.scala	`100% <ø> (ø)`	⬆️

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 5d381ac...aac6509. Read the comment docs.

add Github(None) explanation

juanpedromoreno · 2017-05-16T10:53:55Z

docs/src/main/tut/auth.md

+
+### Get an access token
+
+Requests an access token based on the code retrieved in the [Create a new authorization token](#create-a-new-authorization-token) step of the oAuth process


juanpedromoreno · 2017-05-16T10:57:15Z

docs/src/main/tut/auth.md

+
+See [the API doc](https://developer.github.com/v3/oauth/#web-application-flow) for full reference.
+
+As you can see, a few features of the activity endpoint are missing.


This sentence should be changed, we're not in the activity section, right?

juanpedromoreno · 2017-05-16T10:59:34Z

docs/src/main/tut/git_data.md

+
+Support for `cats.Id`, `cats.Eval` and `Future` (the only supported option for scala-js) are
+provided out of the box when importing `github4s.{js,jvm}.Implicits._`.
+


can we remove the two paragraphs above?

juanpedromoreno · 2017-05-16T10:59:48Z

docs/src/main/tut/git_data.md

+provided out of the box when importing `github4s.{js,jvm}.Implicits._`.
+
+
+## Git Data


juanpedromoreno · 2017-05-16T11:12:33Z

docs/src/main/tut/git_data.md

+See [the API doc](https://developer.github.com/v3/git/blobs/#create-a-blob) for full reference.
+
+## Trees
+


Agreed with @BenFradet . We could some background about trees here:

As you probably know, Git can be considered as a tree structure. Each commit creates a new node in that tree. Even we could assume that all the Git commands or methods provided by the API, are just tools that serve to navigate on this tree and to manipulate it. In the next sections, let's see how Github4s API provides some methods to wrap the Github API.

juanpedromoreno · 2017-05-16T11:13:12Z

docs/src/main/tut/git_data.md

+
+See [the API doc](https://developer.github.com/v3/git/tags/#create-a-tag-object) for full reference.
+
+As you can see, a few features of the repository endpoint are missing.


juanpedromoreno · 2017-05-16T11:14:27Z

docs/src/main/tut/user.md

+```
+
+They also make use of `cats.Id` but any type container implementing `MonadError[M, Throwable]` will


Same here, do we need the next two paragraphs?

juanpedromoreno · 2017-05-16T11:14:38Z

docs/src/main/tut/user.md

+### Get a user
+
+Get information for a particular user


juanpedromoreno · 2017-05-16T11:14:55Z

docs/src/main/tut/user.md

+### Get an authenticated user
+
+Get information of the authenticated user


juanpedromoreno · 2017-05-16T11:41:28Z

This is great, thanks @AdrianRaFo !

Let's wait to @BenFradet for final review though :)

BenFradet

I have a few nits regarding the doc but this looks great! 👍

BenFradet · 2017-05-16T16:38:01Z

docs/src/main/tut/auth.md

@@ -0,0 +1,123 @@
+---
+layout: docs
+title: Authentication API


This is tangential / should be treated in another issue/pr but I wonder if we shouldn't rename the API Authorization instead of Authentication to better stick to what github is saying, what do you think?

Agreed, we could file an issue to tackle it before releasing the next version.

I'm on it 👍

BenFradet · 2017-05-16T16:39:12Z

docs/src/main/tut/auth.md

+**NOTE**: In the examples you will see `Github(None)`
+because if you are authenticating for the first time you don't have any accessToken yet.
+
+## Authentication


This level isn't needed since this API only deals with auth

BenFradet · 2017-05-16T16:39:25Z

docs/src/main/tut/auth.md

+// if you're using ScalaJS, replace occurrences of HttpResponse by SimpleHttpResponse
+//import github4s.js.Implicits._
+//import fr.hmil.roshttp.response.SimpleHttpResponse
+


BenFradet · 2017-05-16T16:39:49Z

docs/src/main/tut/auth.md

+```
+
+**NOTE**: In the examples you will see `Github(None)`
+because if you are authenticating for the first time you don't have any accessToken yet.


access token

BenFradet · 2017-05-16T16:41:27Z

docs/src/main/tut/auth.md

+
+### Authorize a url
+
+Generates the authorize url with a random state, both are returned within an Authorize object.


"an authorized URL" and "[Authorize][auth-scala]"

BenFradet · 2017-05-16T17:07:43Z

docs/src/main/tut/pull_request.md

@@ -100,9 +99,10 @@ createPullRequestData.exec[cats.Id, HttpResponse[String]]() match {
 }
 ```

-On the other hand, we can pass a `issue` id (through `NewPullRequestIssue` object) instead of the title and the body to get this parameter of the issue
+On the other hand, we can pass a `issue` id (through `NewPullRequestIssue` object)
+instead of the title and the body to get this parameter of the issue.


On the other hand, we can pass an issue id (through a NewPullRequestIssue object) instead of the title and body.

BenFradet · 2017-05-16T17:07:58Z

docs/src/main/tut/pull_request.md


-**NOTE**: This option deletes the issue
+**NOTE**: This option deletes the issue.

 ```scala
 val createPullRequestIssue = Github(accessToken).pullRequests.create("47deg", "github4s", NewPullRequestIssue("105"),"my-branch","base-branch",Some(true))


no spaces between params and weird indent

BenFradet · 2017-05-16T17:10:54Z

docs/src/main/tut/user.md

+### Get an authenticated user
+
+Get information of the authenticated user.


I'd add: "making the API call" at the end of the sentence

BenFradet · 2017-05-16T17:12:50Z

github4s/shared/src/main/scala/github4s/api/Users.scala

  def getUsers(
      accessToken: Option[String] = None,
      headers: Map[String, String] = Map(),
      since: Int,
-      pagination: Option[Pagination] = None
+      pagination: Option[Pagination] = Some(httpClient.defaultPagination)


just curious as to why the change?

I just have setted the same default pagination for all APIs

Let's get back to None, I don't see any benefit changing it .

BenFradet · 2017-05-16T17:16:13Z

github4s/shared/src/test/scala/github4s/unit/ReposSpec.scala

@@ -87,6 +108,82 @@ class ReposSpec extends BaseSpec {
    )
  }

+  "Repos.listcommit" should "call to httpClient.get with the right parameters" in {


listCommits

minor fixes

BenFradet · 2017-05-17T09:40:38Z

@AdrianRaFo do you need me to have another look?

AdrianRaFo · 2017-05-17T12:51:27Z

feel free to do it or approve it for merge

BenFradet

Looks great 👍

adrian added 4 commits May 15, 2017 12:08

Fix return in docs

692585a

Create empty docs Fix changelog Add description in integration test

Fix and create docs and test in issue, gitdata and repo

d7becc6

fixes repo test

622bbb0

wip user and auth docs

fb8bfdb

AdrianRaFo requested review from fedefernandez, BenFradet and juanpedromoreno May 15, 2017 15:47

BenFradet reviewed May 15, 2017

View reviewed changes

finish docs

b4c7525

set default pagination

juanpedromoreno requested changes May 16, 2017

View reviewed changes

Fixes in docs

fe14674

delete default pagination

BenFradet reviewed May 16, 2017

View reviewed changes

highlight

05bd85f

add Github(None) explanation

juanpedromoreno requested changes May 16, 2017

View reviewed changes

juanpedromoreno removed the request for review from fedefernandez May 16, 2017 11:18

juanpedromoreno changed the title ~~WIP Missing test and docs~~ Missing Test and Docs May 16, 2017

final changes requested

12ec376

juanpedromoreno approved these changes May 16, 2017

View reviewed changes

minor test change

73aab1a

BenFradet reviewed May 16, 2017

View reviewed changes

adrian added 2 commits May 17, 2017 10:50

docs changes requested

8d97822

minor fixes

minor fixes

aac6509

BenFradet approved these changes May 17, 2017

View reviewed changes

juanpedromoreno merged commit 02be2ca into master May 17, 2017

juanpedromoreno deleted the arf-131-Missing-test-docs branch May 17, 2017 16:42



		Call to request a new authorization given a basic authentication, the returned object Authorization includes an
		access token

		def getAuth: GHIO[GHResponse[User]] = O.getAuthUser(accessToken)

		The `result` on the right is the corresponding [List[User]][user-scala].

		The `result` on the right is the corresponding [List[User]][user-scala].

		See [the API doc](https://developer.github.com/v3/activity/notifications/#set-a-thread-subscription) for full reference.

		See [the API doc](https://developer.github.com/v3/activity/notifications/#set-a-thread-subscription) for full reference.

		As you can see, a few features of the activity endpoint are missing.


		```

		They also make use of `cats.Id` but any type container implementing `MonadError[M, Throwable]` will


		### Create a new authorization token

		Used to request a new auth token given basic authentication


		Used to request a new auth token given basic authentication

		You can create a tree using `newAuth`, it takes as arguments:


		### Get an access token

		Requests an access token based on the code retrieved in the [Create a new authorization token](#create-a-new-authorization-token) step of the oAuth process


		Support for `cats.Id`, `cats.Eval` and `Future` (the only supported option for scala-js) are
		provided out of the box when importing `github4s.{js,jvm}.Implicits._`.

		provided out of the box when importing `github4s.{js,jvm}.Implicits._`.


		## Git Data

		See [the API doc](https://developer.github.com/v3/git/blobs/#create-a-blob) for full reference.

		## Trees


		See [the API doc](https://developer.github.com/v3/git/tags/#create-a-tag-object) for full reference.

		As you can see, a few features of the repository endpoint are missing.

		### Get an authenticated user

		Get information of the authenticated user


		### Authorize a url

		Generates the authorize url with a random state, both are returned within an Authorize object.

Missing Test and Docs #132

Missing Test and Docs #132

Conversation

AdrianRaFo commented May 15, 2017 • edited by juanpedromoreno

BenFradet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdrianRaFo commented May 16, 2017

BenFradet commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenFradet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 16, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanpedromoreno commented May 16, 2017

BenFradet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenFradet commented May 17, 2017

AdrianRaFo commented May 17, 2017 • edited

BenFradet left a comment

Choose a reason for hiding this comment

AdrianRaFo commented May 15, 2017 •

edited by juanpedromoreno

codecov-io commented May 16, 2017 •

edited

AdrianRaFo commented May 17, 2017 •

edited