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.
