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

Refactor Algebras #130

Merged
merged 11 commits into from
May 12, 2017
Merged

Refactor Algebras #130

merged 11 commits into from
May 12, 2017

Conversation

AdrianRaFo
Copy link
Contributor

@AdrianRaFo AdrianRaFo commented May 11, 2017

Fixes #129 Refactor status and notification algebras and their docs.
Create repository documentation

adrian added 5 commits May 11, 2017 13:28
minor doc fixed
minor notification fixes
minor doc fixes
minor style reformat
Docs ready to add repo doc
minor fixes
refactor commit
Copy link
Contributor

@BenFradet BenFradet left a comment

Choose a reason for hiding this comment

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

LGTM, except for a few things in the doc 👍

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

- [Get repository](#get-repository)
Copy link
Contributor

Choose a reason for hiding this comment

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

Get a repository, no?

}
```

The `result` on the right is the created [List[Commit]][commit-scala].
Copy link
Contributor

Choose a reason for hiding this comment

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

created -> corresponding


As a result, if you'd like to see a feature supported, feel free to create an issue and/or a pull request!

[repository-scala]: https://github.com/47deg/github4s/blob/master/github4s/`SHA`red/src/main/scala/github4s/free/domain/Repository.scala
Copy link
Contributor

Choose a reason for hiding this comment

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

the following links are missing / reference nothing:

  • status-scala -> repository-scala
  • commit-scala -> repository-scala
  • user-scala
  • release-scala -> repository-scala
  • content-scala -> repository-scala

@@ -135,7 +135,7 @@ class Repos[C, M[_]](
)

/**
* Fetch contributors list for the the specified repository,
* Fetch contributors listStatus for the the specified repository,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary

@@ -175,7 +175,7 @@ class Repos[C, M[_]](
* @param targetCommitish specifies the commitish value that determines where the Git tag is created from.
* Can be any branch or commit SHA. Unused if the Git tag already exists.
* Default: the repository's default branch (usually `master`).
* @param draft `true` to create a draft (unpublished) release, `false` to create a published one.
* @param draft `true` to createStatus a draft (unpublished) release, `false` to createStatus a published one.
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor

Choose a reason for hiding this comment

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

replace all is not a good idea! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was a problem with refactor rename in intellij, I'm sorry

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah no problem :D

@codecov-io
Copy link

codecov-io commented May 12, 2017

Codecov Report

Merging #130 into master will decrease coverage by 0.1%.
The diff coverage is 87.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #130      +/-   ##
========================================
- Coverage    88.1%    88%   -0.11%     
========================================
  Files          38     36       -2     
  Lines         555    550       -5     
  Branches        2      2              
========================================
- Hits          489    484       -5     
  Misses         66     66
Impacted Files Coverage Δ
...hared/src/main/scala/github4s/api/Activities.scala 100% <ø> (ø)
...thub4s/shared/src/main/scala/github4s/Github.scala 80% <ø> (ø) ⬆️
...c/main/scala/github4s/free/domain/Repository.scala 100% <ø> (ø) ⬆️
...main/scala/github4s/free/algebra/ActivityOps.scala 100% <100%> (ø)
...b4s/shared/src/main/scala/github4s/api/Repos.scala 96.42% <100%> (+0.77%) ⬆️
...4s/shared/src/main/scala/github4s/GithubAPIs.scala 100% <100%> (ø) ⬆️
...ub4s/shared/src/main/scala/github4s/api/Auth.scala 100% <100%> (ø) ⬆️
...in/scala/github4s/free/algebra/RepositoryOps.scala 66.66% <50%> (-8.34%) ⬇️
...cala/github4s/free/interpreters/Interpreters.scala 78.78% <87.5%> (-1.22%) ⬇️

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 0e6041a...42a3275. Read the comment docs.

Copy link
Contributor

@fedefernandez fedefernandez left a comment

Choose a reason for hiding this comment

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

Left some minor comments, outstanding work!
Once addressed, LGTM!

@@ -211,15 +225,15 @@ class Interpreters[M[_], C](
/**
* Lifts Notification Ops to an effect capturing Monad such as Task via natural transformations
Copy link
Contributor

Choose a reason for hiding this comment

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

Notification -> Activity


"Notification." should "call to httpClient.put with the right parameters" in {
"Activity.SetThreadSubscription" should "call to httpClient.put with the right parameters" in {
Copy link
Contributor

Choose a reason for hiding this comment

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

Activity.setThreadSub


"Notifications.SetThreadSubscription" should "call to NotificationOps with the right parameters" in {
"Activities.SetThreadSubscription" should "call to ActivityOps with the right parameters" in {
Copy link
Contributor

Choose a reason for hiding this comment

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

Activities.setThreadSub

@@ -87,4 +87,56 @@ class ReposSpec extends BaseSpec {
)
}

"Repos.get" should "call httpClient.get with the right parameters" in {
Copy link
Contributor

Choose a reason for hiding this comment

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

Repos.getStatus

@fedefernandez fedefernandez merged commit 5d381ac into master May 12, 2017
@fedefernandez fedefernandez deleted the arf-129-Refactor-Algebras branch May 12, 2017 11:04
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