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.

Sign up for GitHub

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

Jump to bottom

Tagless final refactor #344

Merged
merged 52 commits into from
Feb 21, 2020
Merged

Tagless final refactor #344

merged 52 commits into from
Feb 21, 2020

Conversation

bond15
Copy link
Contributor

@bond15 bond15 commented Feb 13, 2020

Everything except mocked server unit tests.

bond15 and others added 30 commits January 21, 2020 09:57
@bond15
initial refactoring from free monad to tagless final
fa918f6
@bond15
Added Algebras
0999a71
@bond15
Refactored HttpClient, temporarily bypassing HttpRequestBuilderExtens…
1da6deb 
…ion, added runtime implicits, added User alg interpretation
@bond15
moving /free/domain to /taglessFinal/domain
714c191
@bond15
Adding interpreters for algebras. Interpreters target the /api classes
3bbd9cf
@bond15
reformatted/linted, remove ADTs from algebras, added more implicit ru…
9c3c9e4 
…ntime instnaces
@bond15
updated algebra signatures to match existing api, removed accessToken…
5dc5eb5 
… as default param, added headers as default param
@bond15
Modified intepreters to fit new algebra definitions and take accessT…
1cbc83e 
…oken as a default parameter
@bond15
removing free monad implementation
0a4abd2
@bond15
updating GHWorkflow and Github
295d91e
@bond15
for Rafa review
0f0ba40
@bond15
pre merge
87c2f7e
@bond15
Resolved merge conflicts
3bedc4e
@bond15
Updated F type constraint F[_]: Applicative -> F[_]: ConcurrentEffect
21a1f07
@bond15
For Rafa comment, progress on replacing scalaj-http with http4s
e949ade
@bond15
For Rafa review and refactor
85c63c1
applied some refactoring to have clean layers
a8ef4fa
@bond15
Merge pull request #333 from 47deg/rafactoring
20ff5d2 
applied some refactoring to have clean layers
@bond15
Adding toFutue and toId syntax for IO[A]
bdc110f
@bond15
removing unit tests associated with deleted classes
36f8c32
@bond15
For Rafa Review, unit tests
52bc263
@bond15
updating unit tests for interpreters
466f5c0
@bond15
unit test linting
a31b48d
@bond15
refactoring integration test to match new api usage
e9c5823
@bond15
updating unit and integration test base spec
20ee2f5
@bond15
linting and updating scala version 2.13 and blaze client
a8de484
@bond15
fixing TestData for integration test
32658ed
@bond15
Updateding FakeResponses for Decoder testing
aa5ee03
@bond15
fix integration test, pull GITHUB4S_ACCESS_TOKEN from sys env
834acb2
upgrades some models
4771d05
@rafaparadela rafaparadela marked this pull request as ready for review February 17, 2020 15:55
Merge branch 'master' into taglessFinalRefactor
2a95927 
# Conflicts:
#	project/ProjectPlugin.scala
BenFradet
BenFradet approved these changes Feb 17, 2020
View reviewed changes
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.

Tremendous amount of work 🔥

docs/docs/git_data.md Outdated Show resolved Hide resolved
docs/docs/issue.md Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/GithubIOSyntax.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/GithubIOSyntax.scala Show resolved Hide resolved
github4s/src/main/scala/github4s/algebras/Auth.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/algebras/PullRequests.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/algebras/PullRequests.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/algebras/PullRequests.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/interpreters/UsersInterpreter.scala Outdated Show resolved Hide resolved
github4s/src/test/scala/github4s/utils/FakeResponses.scala Outdated Show resolved Hide resolved
L-Lavigne
L-Lavigne reviewed Feb 18, 2020
View reviewed changes
Copy link

@L-Lavigne L-Lavigne left a comment

Choose a reason for hiding this comment

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

Excellent work @bond15 ! I only have a few minor stylistic nitpicks.

github4s/src/main/scala/github4s/GithubIOSyntax.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/Github.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/http/Http4sSyntax.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/modules/GithubAPIs.scala Outdated Show resolved Hide resolved
github4s/src/test/scala/github4s/unit/UserSpec.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/interpreters/RepositoriesInterpreter.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/interpreters/OrganizationsInterpreter.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/interpreters/PullRequestsInterpreter.scala Show resolved Hide resolved
github4s/src/main/scala/github4s/interpreters/RepositoriesInterpreter.scala Show resolved Hide resolved
anamariamv
anamariamv approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@anamariamv anamariamv left a comment

Choose a reason for hiding this comment

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

Great work !!! 💯 👌
Minor coment.

One question:
What do you think if we put all the API URLs in a separate file? It would be much easier to read in my opinion.

github4s/src/main/scala/github4s/algebras/GitData.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/algebras/PullRequests.scala Outdated Show resolved Hide resolved
angoglez
angoglez reviewed Feb 18, 2020
View reviewed changes
Copy link
Member

@angoglez angoglez left a comment

Choose a reason for hiding this comment

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

Massive work @bond15 !! 👏
I've left only a few minor comments

@bond15
Copy link
Contributor Author

bond15 commented Feb 18, 2020 •
edited
Loading

Hmm... Seems every few times the integration tests are run, "RemoveLabel" fails.. looking into this.

So far, I am not able to reproduce this error in my local integration tests

juanpedromoreno
juanpedromoreno approved these changes Feb 18, 2020
View reviewed changes
Copy link
Member

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

Outstanding job @bond15 ! Really excited about the diff ❗️

+3,008 −11,076 🎉

github4s/src/main/scala/github4s/Github.scala Show resolved Hide resolved
github4s/src/main/scala/github4s/Github.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/http/RequestBuilder.scala Outdated Show resolved Hide resolved
github4s/src/main/scala/github4s/algebras/Issues.scala Show resolved Hide resolved
github4s/src/main/scala/github4s/interpreters/IssuesInterpreter.scala Show resolved Hide resolved
github4s/src/main/scala/github4s/modules/GithubAPIs.scala Show resolved Hide resolved
github4s/src/test/scala/github4s/integration/GHActivitiesSpec.scala Outdated Show resolved Hide resolved
@rafaparadela
Copy link

Thanks @anamariamv @angoglez @BenFradet @L-Lavigne @juanpedromoreno for this amazing code review.

juanpedromoreno
juanpedromoreno approved these changes Feb 20, 2020
View reviewed changes
Copy link
Member

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

💪 thanks @bond15 !

@bond15 bond15 merged commit 5849584 into master Feb 21, 2020
@calvellido calvellido deleted the taglessFinalRefactor branch February 21, 2020 09:09
This was referenced Feb 24, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaparadela rafaparadela rafaparadela approved these changes

@BenFradet BenFradet BenFradet approved these changes

@angoglez angoglez angoglez left review comments

@anamariamv anamariamv anamariamv approved these changes

@juanpedromoreno juanpedromoreno juanpedromoreno approved these changes

@L-Lavigne L-Lavigne L-Lavigne approved these changes

Assignees

@bond15 bond15

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bond15 @rafaparadela @BenFradet @anamariamv @juanpedromoreno @angoglez @L-Lavigne