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

[Chore] Modernize project setup #622

Merged
merged 22 commits into from Apr 24, 2019

Conversation

@kittinunf
Copy link
Owner

commented Apr 21, 2019

Description

This PR aims to modernize the project gradle usage.

  • Remove all of compile and use implementation counterparts
  • Migrate Gradle to 5.2 instead of 4.10
  • Fix inconsistencies around the projects (move files, take out JSON test from Android module etc.)

Type of change

Check all that apply

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (a change which changes the current internal or external interface)
  • This change requires a documentation update

How Has This Been Tested?

In case you did not include tests describe why you and how you have verified the
changes, with instructions so we can reproduce. If you have added comprehensive
tests for your changes, you may omit this section.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation, if necessary
  • My changes generate no new compiler warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Inspect the bytecode viewer, including reasoning why
@kittinunf

This comment has been minimized.

Copy link
Owner Author

commented Apr 21, 2019

Please ignore CircleCI build for now.

@kittinunf kittinunf self-assigned this Apr 21, 2019
@kittinunf kittinunf added this to In progress in fuel-core via automation Apr 21, 2019
@codecov

This comment has been minimized.

Copy link

commented Apr 21, 2019

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #622      +/-   ##
============================================
- Coverage     71.52%   70.47%   -1.06%     
  Complexity      296      296              
============================================
  Files            54       57       +3     
  Lines          1433     1463      +30     
  Branches        189      192       +3     
============================================
+ Hits           1025     1031       +6     
- Misses          321      341      +20     
- Partials         87       91       +4
Impacted Files Coverage Δ Complexity Δ
.../kotlin/com/github/kittinunf/fuel/json/FuelJson.kt 87.5% <ø> (ø) 2 <0> (?)
.../kittinunf/fuel/android/util/AndroidEnvironment.kt 0% <0%> (ø) 0% <0%> (?)
...com/github/kittinunf/fuel/livedata/FuelLiveData.kt 0% <0%> (ø) 0% <0%> (?)
...ava/com/github/kittinunf/fuel/stetho/StethoHook.kt 45.45% <0%> (ø) 0% <0%> (?)

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 70b08e0...a915ff1. Read the comment docs.

@kittinunf kittinunf requested review from iNoles, NikkyAI and SleeplessByte Apr 21, 2019
Copy link
Collaborator

left a comment

i am not sure if implementation is a good idea on all the thing, it will not make the libraries available to consumers, eg: using fuel-coroutines without coroutines
in that case api is more useful

gradlew Outdated Show resolved Hide resolved
@kittinunf

This comment has been minimized.

Copy link
Owner Author

commented Apr 21, 2019

@NikkyAI Agree I will use api for fuel-coroutines

@SleeplessByte

This comment has been minimized.

Copy link
Collaborator

commented Apr 21, 2019

Apart from Nikkys comments, lgtm

@kittinunf kittinunf force-pushed the kv/modernize-project-setup branch from f99e850 to ea05f25 Apr 21, 2019
@kittinunf

This comment has been minimized.

Copy link
Owner Author

commented Apr 21, 2019

It looks like when updated to Gradle 5.2, it will break the test 🤔 🤦‍♂️

@kittinunf kittinunf force-pushed the kv/modernize-project-setup branch from da006c6 to 75b11f2 Apr 22, 2019
@iNoles

This comment has been minimized.

Copy link
Collaborator

commented Apr 22, 2019

"To honour the JVM settings for this build a new JVM will be forked. Please consider using the daemon: https://docs.gradle.org/5.2/userguide/gradle_daemon.html."

fuel-core automation moved this from In progress to Reviewer approved Apr 22, 2019
Copy link
Collaborator

left a comment

seems good to me then

@kittinunf

This comment has been minimized.

Copy link
Owner Author

commented Apr 23, 2019

@iNoles I thought we set it here org.gradle.daemon=true, daemon should be enabled by default no?

@@ -8,20 +7,24 @@ plugins {
}

dependencies {
implementation(Kotlin.stdlib)
// fuel related libraries

This comment has been minimized.

Copy link
@iNoles

iNoles Apr 23, 2019

Collaborator

these should be implementation because api should be only in the libraries.

This comment has been minimized.

Copy link
@kittinunf

kittinunf Apr 23, 2019

Author Owner

it is a sample app, so ... 🤷‍♂️.

This comment has been minimized.

Copy link
@kittinunf

kittinunf Apr 23, 2019

Author Owner

Addressed!

kittinunf added 5 commits Apr 23, 2019
@kittinunf kittinunf merged commit 572d907 into master Apr 24, 2019
2 of 3 checks passed
2 of 3 checks passed
ci/circleci: build Your tests failed on CircleCI
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
fuel-core automation moved this from Reviewer approved to Done Apr 24, 2019
@kittinunf kittinunf deleted the kv/modernize-project-setup branch Apr 24, 2019
@kittinunf kittinunf referenced this pull request May 12, 2019
3 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
fuel-core
  
Done
4 participants
You can’t perform that action at this time.