Yigit/move fetcher factories to fetcher #168

yigit · 2020-05-11T21:48:13Z

No description provided.

store-rx2/src/main/kotlin/com/dropbox/store/rx2/RxFetcher.kt

store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt

README.md

Fetcher factories were global methods, which made them hard to discover since IDE cannot easily auto-complete. This PR moves them into the companion of Fetcher while also making Fetcher a real interface instead of a typealias. Even though it is a bit more code for the developer, now they can easily discover how to create a Fetcher by typing Fetcher. Fixes: #167

Create Fetcher.fromFlow for the flowing version. Rename both SourceOfTruth builder methods to . Rely on param names to disambiguate

yigit · 2020-05-26T04:56:16Z

pushed another attempt. now SoT has from for both methods (relying on method names to disambiguate). Removed stream from fetchers.

Fetcher.from -> suspend fun T
Fetcher.fromFlow -> Flow<T>
Fetcher.fromFetcherResult -> suspend fun FetcherResult<T>
Fetcher.fromFetcherResutlFlow -> Flow<FetcherResult>

The name for the flowable reader in SoT still sounds weird (flowReader). looking for suggestions.

I'm still not super happy but idk what to do and i really think we should avoid the global fetcher builders as they are hard to discover. So I strongly believe we need a solution, i'm not super confident this is it.

eyalgu · 2020-05-26T15:40:44Z

pushed another attempt. now SoT has from for both methods (relying on method names to disambiguate). Removed stream from fetchers.

Fetcher.from -> suspend fun T
Fetcher.fromFlow -> Flow<T>
Fetcher.fromFetcherResult -> suspend fun FetcherResult<T>
Fetcher.fromFetcherResutlFlow -> Flow<FetcherResult>

The name for the flowable reader in SoT still sounds weird (flowReader). looking for suggestions.

I'm still not super happy but idk what to do and i really think we should avoid the global fetcher builders as they are hard to discover. So I strongly believe we need a solution, i'm not super confident this is it.

What are your thought about making from and fromFetcherResult vs fromValues and from? The more generic case seems to be the one that takes FetcherResult while the other one seems to be a connivance (though the more common use case admittedly).

One the one hand using from for the generic use case feels more future proof. On the other hand using from for the common use case make the API clearer right now.

store-rx2/src/main/kotlin/com/dropbox/store/rx2/RxFetcher.kt

store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt

eyalgu · 2020-05-26T15:53:24Z

store/src/main/java/com/dropbox/android/external/store4/SourceOfTruth.kt

         * @param deleteAll function for deleting all records in the source of truth
         */
-        fun <Key : Any, Input : Any, Output : Any> fromNonFlow(
+        fun <Key : Any, Input : Any, Output : Any> from(


After reading through your diff and thinking more, I think that your original approach was right - lets use the names to point people to the most common defaults and the the RTFM if they need something different. In this case I'm landing on the side that the confusion of having name overload with different parameter names outweighs the consistency benefits.

to make sure i understand correctly, you mean keep SoT methods as from for both cases? (like in this CL?)

no. I've come around to you're original approach of naming the SoT methods from and fromNonFlow Overload separation by named properties seems very subtle. It is what you would do in Swift, but there the names are mandatory, not optional.

yigit · 2020-05-26T18:09:49Z

pushed another attempt. now SoT has from for both methods (relying on method names to disambiguate). Removed stream from fetchers.
Fetcher.from -> suspend fun T
Fetcher.fromFlow -> Flow<T>
Fetcher.fromFetcherResult -> suspend fun FetcherResult<T>
Fetcher.fromFetcherResutlFlow -> Flow<FetcherResult>
The name for the flowable reader in SoT still sounds weird (flowReader). looking for suggestions.
I'm still not super happy but idk what to do and i really think we should avoid the global fetcher builders as they are hard to discover. So I strongly believe we need a solution, i'm not super confident this is it.

What are your thought about making from and fromFetcherResult vs fromValues and from? The more generic case seems to be the one that takes FetcherResult while the other one seems to be a connivance (though the more common use case admittedly).

One the one hand using from for the generic use case feels more future proof. On the other hand using from for the common use case make the API clearer right now.

i was thinking the most common case should have the best name hence i thought fromValues should be from. That being said, fromFetcherResult sounds very ugly :/.

We should probably get rid of StoreBuilder.from and make it Store.builder()

eyalgu · 2020-06-03T15:36:27Z

README.md

-            reader = db.postDao()::loadPosts,
+        fetcher = Fetcher.of { api.fetchSubreddit(it, "10").data.children.map(::toPosts) },
+        sourceOfTruth = SourceOfTrue.of(
+            flowReader = db.postDao()::loadPosts,


I'm still not sure how discoverable it is to use parameter names do distinguish between the 2 functions. what happens if you call it without parameter names?

without names this does work:

sourceOfTruth = SourceOfTruth.of( db.postDao()::loadPosts, db.postDao()::insertPosts, db.postDao()::clearFeedBySubredditName, db.postDao()::clearAllFeeds )

While this doesn't:

sourceOfTruth = SourceOfTruth.of( { db.postDao().loadPosts(it) }, db.postDao()::insertPosts, db.postDao()::clearFeedBySubredditName, db.postDao()::clearAllFeeds )

re-naming: part of the challange is that this receives 4 methods, only 1 parameter's type is different so ofFlow wouldn't look very good imo either :/.

store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt

eyalgu · 2020-06-03T15:44:22Z

store/src/main/java/com/dropbox/android/external/store4/SourceOfTruth.kt

-            reader: (Key) -> Flow<Output?>,
+        @JvmName("ofFlow")
+        fun <Key : Any, Input : Any, Output : Any> of(
+            flowReader: (Key) -> Flow<Output?>,


assuming you land on "this is discoverable" should we call the flow one reader and the non flow one nonFlowReader to highlight the better default?

updated. i hate it for anyone needs to call it but i think it is the right call.

These appeared after i rebased, missed them completely. Also fixed some tests, appearantly IJ parameter name refactor does not always work

without this, multicaster cannot resolve to the base StoreResponse type

…fetcher' into yigit/move-fetcher-factories-to-fetcher # Conflicts: # store-rx2/src/test/kotlin/com/dropbox/store/rx2/test/RxFlowableStoreTest.kt

codecov · 2020-06-13T18:20:40Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a5c29ff). Click here to learn what that means.
The diff coverage is 85.71%.

@@            Coverage Diff            @@
##             master     #168   +/-   ##
=========================================
  Coverage          ?   86.01%           
  Complexity        ?      230           
=========================================
  Files             ?       54           
  Lines             ?      951           
  Branches          ?      149           
=========================================
  Hits              ?      818           
  Misses            ?       76           
  Partials          ?       57

Impacted Files	Coverage Δ	Complexity Δ
.../android/external/store4/impl/FetcherController.kt	`100.00% <ø> (ø)`	`6.00 <0.00> (?)`
...src/main/kotlin/com/dropbox/store/rx2/RxFetcher.kt	`50.00% <50.00%> (ø)`	`0.00 <0.00> (?)`
...src/main/kotlin/com/dropbox/store/rx3/RxFetcher.kt	`50.00% <50.00%> (ø)`	`0.00 <0.00> (?)`
...in/kotlin/com/dropbox/store/rx2/RxSourceOfTruth.kt	`70.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
...in/kotlin/com/dropbox/store/rx3/RxSourceOfTruth.kt	`70.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
...ava/com/dropbox/android/external/store4/Fetcher.kt	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
...m/dropbox/android/external/store4/SourceOfTruth.kt	`88.23% <100.00%> (ø)`	`0.00 <0.00> (?)`
...va/com/dropbox/android/external/fs3/FSAllEraser.kt	`100.00% <0.00%> (ø)`	`2.00% <0.00%> (?%)`
... and 51 more

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 a5c29ff...fdb26ba. Read the comment docs.

* Move Fetcher factories into companion Fetcher factories were global methods, which made them hard to discover since IDE cannot easily auto-complete. This PR moves them into the companion of Fetcher while also making Fetcher a real interface instead of a typealias. Even though it is a bit more code for the developer, now they can easily discover how to create a Fetcher by typing Fetcher. Fixes: #167 * make rx methods start w/ from too for consistency * Rename fether factories to be more clear, hopefully :/ * remove fetch method, use invoke instead * Make Fetcher.from the one that receives a suspend fun. Create Fetcher.fromFlow for the flowing version. Rename both SourceOfTruth builder methods to . Rely on param names to disambiguate * use .of instead, this seems better to me. We should probably get rid of StoreBuilder.from and make it Store.builder() * fix jvm name for SourceOfTruth.of with flow function * fix RxSourceOfTruth name to match original class * specify bounds for FactoryFetcher * updates per PR review * update graph per SoT rename * update rxjava3 APIs as well These appeared after i rebased, missed them completely. Also fixed some tests, appearantly IJ parameter name refactor does not always work * supress wrong unnecessary cast warning without this, multicaster cannot resolve to the base StoreResponse type * upgade gradle, try to fix build by disabling caching * split subscribers * resubscribe Co-authored-by: miken <[email protected]>

yigit · 2020-06-19T03:52:07Z

oh looks like this got merged into the master branch not main.

* Move Fetcher factories into companion Fetcher factories were global methods, which made them hard to discover since IDE cannot easily auto-complete. This PR moves them into the companion of Fetcher while also making Fetcher a real interface instead of a typealias. Even though it is a bit more code for the developer, now they can easily discover how to create a Fetcher by typing Fetcher. Fixes: #167 * make rx methods start w/ from too for consistency * Rename fether factories to be more clear, hopefully :/ * remove fetch method, use invoke instead * Make Fetcher.from the one that receives a suspend fun. Create Fetcher.fromFlow for the flowing version. Rename both SourceOfTruth builder methods to . Rely on param names to disambiguate * use .of instead, this seems better to me. We should probably get rid of StoreBuilder.from and make it Store.builder() * fix jvm name for SourceOfTruth.of with flow function * fix RxSourceOfTruth name to match original class * specify bounds for FactoryFetcher * updates per PR review * update graph per SoT rename * update rxjava3 APIs as well These appeared after i rebased, missed them completely. Also fixed some tests, appearantly IJ parameter name refactor does not always work * supress wrong unnecessary cast warning without this, multicaster cannot resolve to the base StoreResponse type * upgade gradle, try to fix build by disabling caching * split subscribers * resubscribe Co-authored-by: miken <[email protected]> Co-authored-by: miken <[email protected]>

…obileNativeFoundation#181) * Move Fetcher factories into companion Fetcher factories were global methods, which made them hard to discover since IDE cannot easily auto-complete. This PR moves them into the companion of Fetcher while also making Fetcher a real interface instead of a typealias. Even though it is a bit more code for the developer, now they can easily discover how to create a Fetcher by typing Fetcher. Fixes: MobileNativeFoundation#167 * make rx methods start w/ from too for consistency * Rename fether factories to be more clear, hopefully :/ * remove fetch method, use invoke instead * Make Fetcher.from the one that receives a suspend fun. Create Fetcher.fromFlow for the flowing version. Rename both SourceOfTruth builder methods to . Rely on param names to disambiguate * use .of instead, this seems better to me. We should probably get rid of StoreBuilder.from and make it Store.builder() * fix jvm name for SourceOfTruth.of with flow function * fix RxSourceOfTruth name to match original class * specify bounds for FactoryFetcher * updates per PR review * update graph per SoT rename * update rxjava3 APIs as well These appeared after i rebased, missed them completely. Also fixed some tests, appearantly IJ parameter name refactor does not always work * supress wrong unnecessary cast warning without this, multicaster cannot resolve to the base StoreResponse type * upgade gradle, try to fix build by disabling caching * split subscribers * resubscribe Co-authored-by: miken <[email protected]> Co-authored-by: miken <[email protected]>

yigit requested review from digitalbuddha and eyalgu May 11, 2020 21:48

eyalgu suggested changes May 11, 2020

View reviewed changes

eyalgu reviewed May 12, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

yigit added 5 commits May 25, 2020 21:17

make rx methods start w/ from too for consistency

3345836

Rename fether factories to be more clear, hopefully :/

b51e2b0

remove fetch method, use invoke instead

d7b0970

Make Fetcher.from the one that receives a suspend fun.

9a56e8e

Create Fetcher.fromFlow for the flowing version. Rename both SourceOfTruth builder methods to . Rely on param names to disambiguate

yigit force-pushed the yigit/move-fetcher-factories-to-fetcher branch from f90cf5d to 9a56e8e Compare May 26, 2020 04:56

eyalgu reviewed May 26, 2020

View reviewed changes

yigit added 4 commits June 2, 2020 21:56

use .of instead, this seems better to me.

e1bce0b

We should probably get rid of StoreBuilder.from and make it Store.builder()

fix jvm name for SourceOfTruth.of with flow function

f3b6fc4

fix RxSourceOfTruth name to match original class

220f771

specify bounds for FactoryFetcher

7a2ad14

eyalgu reviewed Jun 3, 2020

View reviewed changes

updates per PR review

a79f652

eyalgu previously approved these changes Jun 5, 2020

View reviewed changes

Merge branch 'master' into yigit/move-fetcher-factories-to-fetcher

de9a916

yigit dismissed eyalgu’s stale review via de9a916 June 5, 2020 14:37

update graph per SoT rename

408b22f

digitalbuddha previously approved these changes Jun 5, 2020

View reviewed changes

update rxjava3 APIs as well

705e9c1

These appeared after i rebased, missed them completely. Also fixed some tests, appearantly IJ parameter name refactor does not always work

yigit dismissed digitalbuddha’s stale review via 705e9c1 June 7, 2020 21:31

yigit and others added 3 commits June 7, 2020 14:47

supress wrong unnecessary cast warning

248ccc0

without this, multicaster cannot resolve to the base StoreResponse type

upgade gradle, try to fix build by disabling caching

b3d7c9a

split subscribers

510214d

miken added 2 commits June 13, 2020 14:14

resubscribe

989cbf4

Merge remote-tracking branch 'origin/yigit/move-fetcher-factories-to-…

fdb26ba

…fetcher' into yigit/move-fetcher-factories-to-fetcher # Conflicts: # store-rx2/src/test/kotlin/com/dropbox/store/rx2/test/RxFlowableStoreTest.kt

digitalbuddha approved these changes Jun 13, 2020

View reviewed changes

eyalgu merged commit 1a954e4 into master Jun 16, 2020

eyalgu deleted the yigit/move-fetcher-factories-to-fetcher branch June 16, 2020 23:52

Yigit/move fetcher factories to fetcher #168

Yigit/move fetcher factories to fetcher #168

Uh oh!

Conversation

yigit commented May 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yigit commented May 26, 2020

Uh oh!

eyalgu commented May 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eyalgu May 26, 2020

Choose a reason for hiding this comment

Uh oh!

yigit May 26, 2020

Choose a reason for hiding this comment

Uh oh!

eyalgu May 26, 2020

Choose a reason for hiding this comment

Uh oh!

yigit commented May 26, 2020

Uh oh!

eyalgu Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

yigit Jun 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eyalgu Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

yigit Jun 5, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yigit commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jun 13, 2020 •

edited

Loading