search

tgrapperon / swift-dependencies-additions

More dependencies for `swift-dependencies`
MIT License
298 stars 39 forks source link

feat: Ability to set a fetch limit for a fetch request #58

Closed Semty closed 1 year ago

Semty commented 1 year ago

Just added a small adjustment to the public interface to be able to set a fetch request limit

tgrapperon commented 1 year ago

Hey @Semty! Thanks for the PR! That makes sense indeed. That being said, it raises the question of fetchOffset and fetchBatchSize, but I'm a little wary that that'll make a lot of arguments to the function. And there are other properties that can be set too. I'm wondering if having an optional trailing configure: (NSFetchRequest) -> Void closure wouldn't be more flexible? The problem is that it'll allow users to mess with the return type and override the predicate/sort descriptors, so I'm wondering if passing an inout FetchRequestConfiguration that we'd define with only a subset of what's configurable wouldn't be better. What do you think about it?

Semty commented 1 year ago

Hey @Semty! Thanks for the PR!

That makes sense indeed. That being said, it raises the question of fetchOffset and fetchBatchSize, but I'm a little wary that that'll make a lot of arguments to the function. And there are other properties that can be set too.

I'm wondering if having an optional trailing configure: (NSFetchRequest) -> Void closure wouldn't be more flexible? The problem is that it'll allow users to mess with the return type and override the predicate/sort descriptors, so I'm wondering if passing an inout FetchRequestConfiguration that we'd define with only a subset of what's configurable wouldn't be better. What do you think about it?

Hey, @tgrapperon! 100% agree with you here, but we need to decide what to do with some basic variables such as 'predicate' and 'sortDescriptors'. Right now I see three options:

  1. Leave them as they are and write another function that will take a new non-optional trailing closure
  2. Leave them as they are and just add a new optional closure as an added function parameter
  3. Remove them and add only an optional closure to make it specific that you need to specify configuration for the request if you want to customise it

What do you think?

tgrapperon commented 1 year ago

I think that the predicate and sortDescriptor fields should stay exposed, as they are in SwiftUI's @FetchRequest. One alternative solution would be to also accept a fully fledged NSFetchRequest as parameter. As this type is miraculously generic, we can lock its type to be a NSManagedObject, and we can regularize its sort descriptors if needed. This would offer users total control over the kind of request they want to pass. I'll toy with the idea and I'll let you know.

tgrapperon commented 1 year ago

Hey @Semty! I've updated the branch with variants that are expecting a NSFetchRequest argument. It simply makes sure that there is at least a sort descriptor (albeit this was a requirement from using NSFetchedRequestController, and the library doesn't use that to observe the fetch, so we can maybe drop them at some point (but this is another question)). Let me know if it solves your issues.

Semty commented 1 year ago

hey, @tgrapperon! Yes, It's pretty much solved my problems, thank you so much! Hence, I hereby close this PR as no longer relevant 👌