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

Kollect 0.1 - Mirrors Fetch redesign #14

Merged
merged 147 commits into from
Feb 4, 2019
Merged

Kollect 0.1 - Mirrors Fetch redesign #14

merged 147 commits into from
Feb 4, 2019

Conversation

JorgeCastilloPrz
Copy link
Contributor

@JorgeCastilloPrz JorgeCastilloPrz commented Sep 18, 2018

Please read this description before stepping in:

Here you have a complete 1 to 1 port of Fetch redesign. All the features are present, also the tests. This is completely functional and should be production ready, assuming Fetch also is.

Added every single test on Fetch to this code base also, so we have tests covering batching, de-duplication, asynchrony, concurrency, error handling, syntax, and much more.

Also, sorry for the huge PR! 😅 it was much easier to do it like this, since before this point the repo just had some basic scaffoldings, nothing complete or working. I followed @purrgrammer's Fetch redesign PR style to try to shortcut a lot of intermediate discussions that could arise without a final runnable version.

My advice for reviewing this would be:

  • Given the previous state of this repo, let's try to not be super blocker about things that could be iterated separately (e.g: moving all bindings to fx, using more idiomatic Kotlin code, etc). Let's just keep our eyes on the actual features. The goal of this redesign is to mirror Fetch features 1 to 1.
  • My goal here is to reach a feature complete + test coverage starting point we can use to iterate over, following @purrgrammer's Fetch redesign PR style, so any suggestion about new features not present in Fetch already should also be pulled to different PRs.
  • Use Fetch docs to understand the library features as I did, that's gonna be helpful.

Structure

Segregated the lib into different modules: kollect-core, kollect-extensions, kollect-typeclasses, mostly because of extensions requiring to be on a separate module to work. There's also a kollect-test with all the tests. That pushed me to pull out typeclasses too, so all the three modules can use them. Still, typeclasses are Timer and Clock, which will eventually be moved to Arrow.

The structure kind of mimics what we've been doing for arrow, more or less.

Caveats

  • I approached caches as in Fetch, where I (index) and A (result) types are typed as Any. I tried to move those types to be actual gneric type args but that crawled the whole hierarchy up to the client call sites, where you'd be enforced to do Kollect.run<F, I, A>(concurrentF), which looked a bit verbose to me. Still, I'm totally open for suggestions there, and I would love to know why it's done in Fetch the way it is.

I'd love to get review from:

  • @purrgrammer given you worked on Fetch's redesign, your eyes would be the perfect ones for this task.
  • @nomisRev because you showed interest on taking a look at all this
  • @pakoito or @raulraja to have another Arrow member on this.

nomisRev and others added 4 commits February 3, 2019 15:50
Co-Authored-By: JorgeCastilloPrz <jorge.castillo.prz@gmail.com>
Co-Authored-By: JorgeCastilloPrz <jorge.castillo.prz@gmail.com>
@JorgeCastilloPrz
Copy link
Contributor Author

Addressed all your comments @nomisRev. Thanks 👍

@JorgeCastilloPrz
Copy link
Contributor Author

Addressed all your comments now @nomisRev. Sorry for missing those out, seems like GitHub is hiding some comments inside the diff and not exposing them front page.

@JorgeCastilloPrz
Copy link
Contributor Author

JorgeCastilloPrz commented Feb 3, 2019

@purrgrammer , after applying @nomisRev suggestion on 5d54554, I got KollectMonad segregated into KollectMonad, KollectApplicative, KollectFunctor. (in Arrow we're enforced to provide all the extensions separately).

The thing is that tests got broken because I wasn't implementing ap yet for KollectApplicative neither for KollectMonad. So I added those methods and delegated them into our Kollect datatype ap() method, the same way we do for the rest of the combinators usually. That got tests fixed but required a big // TODO() in Kollect#ap().

Now I'm clueless about how to implement Kollect#ap(). Does it make sense to provide Functor or Applicative instances for Kollect? In that case, could you help on implementing ap()? It's okay if it's in Scala.

@purrgrammer
Copy link

Now I'm clueless about how to implement Kollect#ap(). Does it make sense to provide Functor or Applicative instances for Kollect? In that case, could you help on implementing ap()? It's okay if it's in Scala.

We've discussed this privately on Slack, ap can be formulated in terms of product:

ap(ff, fa) = product(ff, fa).map({ case (f, a) => f(a) })

@JorgeCastilloPrz
Copy link
Contributor Author

Alright, ap has been implemented by delegating it to product, as you suggested @purrgrammer.

Copy link
Contributor

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

Awesome job! 👏

@raulraja raulraja merged commit a72ee95 into master Feb 4, 2019
@raulraja raulraja deleted the jc-kollect-redesign branch February 4, 2019 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants