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

Issue #493: Use WorkManager in fretboard #503

Closed

Conversation

Projects
None yet
2 participants
@fercarcedo
Copy link
Contributor

commented Jul 25, 2018

This pull request adds a possible implementation of a WorkManager-based scheduler for Fretboard. However, it is using the latest version (which is currently an alpha, 1.0.0-alpha05), and it adds a lot of transitive dependencies (as detailed by @pocmo on #493)

Closes #493

@codecov

This comment has been minimized.

Copy link

commented Jul 25, 2018

Codecov Report

Merging #503 into master will decrease coverage by 0.26%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #503      +/-   ##
============================================
- Coverage      76.8%   76.53%   -0.27%     
  Complexity      942      942              
============================================
  Files           146      148       +2     
  Lines          3449     3461      +12     
  Branches        489      489              
============================================
  Hits           2649     2649              
- Misses          549      561      +12     
  Partials        251      251
Impacted Files Coverage Δ Complexity Δ
.../scheduler/workmanager/WorkManagerSyncScheduler.kt 0% <0%> (ø) 0 <0> (?)
...vice/fretboard/scheduler/workmanager/SyncWorker.kt 0% <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 20f5ba1...e9df3eb. Read the comment docs.

@pocmo
Copy link
Contributor

left a comment

Nice. That's not much code. :)

@@ -25,6 +25,8 @@ dependencies {
implementation project(':support-ktx')
implementation "org.jetbrains.kotlin:kotlin-stdlib:${rootProject.ext.dependencies['kotlin']}"
implementation "org.jetbrains.kotlinx:kotlinx-coroutines-android:${rootProject.ext.dependencies['coroutines']}"
implementation "android.arch.work:work-runtime:${rootProject.ext.dependencies['workmanager']}"
implementation "android.arch.work:work-firebase:${rootProject.ext.dependencies['workmanager']}"

This comment has been minimized.

Copy link
@pocmo

pocmo Jul 25, 2018

Contributor

Is that needed?

If yes: Can you comment on the issue about that? And can you run the androidDependencies task and see what else this pulls in? Especially I'm interested in knowing whether this requires closed-source Firebase dependencies.

@pocmo pocmo added the 🛑 blocked label Jul 26, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

Blocked on:

  • Non-alpha release of work manager library
  • Do we need to depend on work-firebase and what transitive dependencies does this have?
@fercarcedo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

Actually, we don't need the work-firebase dependency, since it only provides benefits for API < 21 devices, and we have minSdk 21, so I have removed it from the pull request.

@fercarcedo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

These are the transitive dependencies of WorkManager:

\--- android.arch.work:work-runtime:1.0.0-alpha05
     +--- android.arch.lifecycle:extensions:1.1.0
     |    +--- android.arch.lifecycle:runtime:1.1.0 (*)
     |    +--- android.arch.core:common:1.1.0 -> 1.1.1 (*)
     |    +--- android.arch.core:runtime:1.1.0 -> 1.1.1
     |    |    +--- com.android.support:support-annotations:26.1.0 -> 27.1.1
     |    |    \--- android.arch.core:common:1.1.1 (*)
     |    +--- com.android.support:support-fragment:26.1.0
     |    |    +--- com.android.support:support-compat:26.1.0 -> 27.1.1 (*)
     |    |    +--- com.android.support:support-core-ui:26.1.0
     |    |    |    +--- com.android.support:support-annotations:26.1.0 -> 27.1.1
     |    |    |    \--- com.android.support:support-compat:26.1.0 -> 27.1.1 (*)
     |    |    \--- com.android.support:support-core-utils:26.1.0
     |    |         +--- com.android.support:support-annotations:26.1.0 -> 27.1.1
     |    |         \--- com.android.support:support-compat:26.1.0 -> 27.1.1 (*)
     |    +--- android.arch.lifecycle:common:1.1.0 (*)
     |    +--- android.arch.lifecycle:livedata:1.1.0
     |    |    +--- android.arch.core:runtime:1.1.0 -> 1.1.1 (*)
     |    |    +--- android.arch.lifecycle:livedata-core:1.1.0
     |    |    |    +--- android.arch.lifecycle:common:1.1.0 (*)
     |    |    |    +--- android.arch.core:common:1.1.0 -> 1.1.1 (*)
     |    |    |    \--- android.arch.core:runtime:1.1.0 -> 1.1.1 (*)
     |    |    \--- android.arch.core:common:1.1.0 -> 1.1.1 (*)
     |    \--- android.arch.lifecycle:viewmodel:1.1.0
     |         \--- com.android.support:support-annotations:26.1.0 -> 27.1.1
     \--- android.arch.persistence.room:runtime:1.1.1-rc1
          +--- android.arch.persistence.room:common:1.1.1-rc1
          |    \--- com.android.support:support-annotations:26.1.0 -> 27.1.1
          +--- android.arch.persistence:db-framework:1.1.1-rc1
          |    +--- com.android.support:support-annotations:26.1.0 -> 27.1.1
          |    \--- android.arch.persistence:db:1.1.1-rc1
          |         \--- com.android.support:support-annotations:26.1.0 -> 27.1.1
          +--- android.arch.persistence:db:1.1.1-rc1 (*)
          +--- android.arch.core:runtime:1.1.1 (*)
          \--- com.android.support:support-core-utils:26.1.0 (*)

@fercarcedo fercarcedo force-pushed the fercarcedo:fretboard-workmanager branch from effd478 to 39bf674 Jul 26, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

Actually, we don't need the work-firebase dependency, since it only provides benefits for API < 21

Great! :)

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

These are the transitive dependencies of WorkManager:

It's nice that this only contains a workmanager alpha and no other android x/alpha dependency.

Issue #493: Use WorkManager in fretboard
Closes #493: Use WorkManager in fretboard

@fercarcedo fercarcedo force-pushed the fercarcedo:fretboard-workmanager branch from 39bf674 to e9df3eb Aug 3, 2018

@fercarcedo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

Now it's updated to the new WorkManager 1.0.0-alpha06

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

@fercarcedo Did anything change regarding the transitive dependencies?

@fercarcedo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

No, the dependencies are still the same

@pocmo pocmo removed the 🛑 blocked label Sep 24, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Landed as 95f801e

@pocmo pocmo closed this Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.