Thanks for iterating on this - from my point of view the API looks quite good.
Thanks, Robert On Thu, 2018-08-23 at 11:37 -0400, Jason E Bailey wrote: > I appreciate the more in depth feedback, when you've worked on > something for a while it's hard to see it from a different point of > view. > > - Jason > > On Thu, Aug 23, 2018, at 5:19 AM, Robert Munteanu wrote: > > My notes are as follows: > > > > a. Do we need custom exceptions? I.e., do we expect consumers to > > try/catch when parsing and then do something about the error? If > > yes, > > we keep the exception, otherwise we could resort to something like > > an > > IllegalArgumentException > > That's a solid concept on when to implement a custom exception. I > think it streamlines it to make it an IllegalArgumentException > > > b. The ResourceFilter seems to conflate building instances > > (parse()) > > and accumulating parameters (addParameter). Not a big deal but can > > be a > > bit hard to parse at first ( excuse the pun ). > > > > Typically I'd see these as builder classes, with an API such as > > > > ResourcePredicates.withParameter(...).parse(....) > > I'll address this below. > > > c. The ResourceFilterStream is an almost 1:1 wrapper over the > > ResourceStream. Do we really need both? > > Yes. I've bounced back and forth enough between combining them and > separating them that I feel I have a good grasp on this one. The > ResourceStream has a very specific function of creating > Stream<Resource> objects. Ideally it shouldn't even be it's own class > and at some point these methods will be moved into the Resource > api. With that in mind, in the future, I would deprecate this class > and not methods in a combined ResourceStream/ ResourceFilterStream. > Additionally the ResourceFilterStream is doing something different > where it's maintaining state and allows you to define a branch > selector and child selector. Having this combined makes it cleaner to > construct code that will find specific resources. Otherwise it starts > to get unwieldy. > > > d. Adapting a resource to a ResourceFilter is a bit confusing, as > > the > > ResourceFilter does not have any state based on the resource it > > adapts > > from. This also tied into b. above. > > Agreed, this is the part that I'm having the most problem with, > because I'm trying not to expose any of the internal classes. My > options as far as I'm aware is to use adaptTo or a service. The > adapter doesn't feel right as you've pointed out, but requires less > exposure of classes. When using a Service, I have to create a factory > which is better but still feels a bit odd since it just wraps a new > ResourceFilterImpl(). I played around with doing an OSGi factory > implementation so that the Reference would be a unique instance each > time but that is really dependent on forming the Reference annotation > correctly, which is something I also feel is awkward. > > I'll tackle the Builder pattern again and see what I come up with. > > >
