Hi Bertrand,
On Fri, Dec 27, 2013 at 5:48 AM, Bertrand Delacretaz <[email protected]> wrote: > Hi Justin, > > Finally found time to review YAMF. > > On Thu, Dec 19, 2013 at 6:07 PM, Justin Edelson > <[email protected]> wrote: >> ...I'm calling this YAMF for now, although ideally >> we'll just call it Sling Models... > > +1 for Sling Models, that's consistent with our general naming. > > My comments, after looking at the wiki page only: > > 1) Should it be @SlingModel rather than @Model? By analogy with @SlingServlet. I thought about this, but honestly the "Sling" part here is duplicative. sling is already part of the package name, so why include it in the annotation name as well? > > 2) For adaptation, would my SLING-2938 adapter methods prototype make sense? Possibly, although as I wrote on the wiki page, one of the design goals was to not require changes to existing Sling bundles which AFAICT SLING-2938 does. > > 3) Is it a good idea to use @Inject for both services and resource > values? Using @ResourceValue for the latter makes it easier to explain > both IMO. There are already 5 injectors available and I would guess that we need at least one or two others in the core. And other injectors can be added by downstream projects. I don't think it makes sense to have injector-specific annotations unless absolutely necessary (@Filter being the one obvious example of where an annotation is injector-specific). > > 4) Also, would @Path make sense instead of @Named? Does the > implementation then support @Path("child-node/some-property") ? See above. The idea (beyond the goal of using standard annotations where possible) is that @Named works across injector types. @Path doesn't make sense for a binding value or request attribute. > > 5) @Projection is not obvious to me from its name, how about @ChildPath ? Yeah, I'm not thrilled with @Projection either, but for the same reasons as above @ChildPath won't work consistently. How about @From as in @Model(adaptables = Resource.class) public class Model { @Inject @From("parent") @Named("jcr:title") private String parentTitle; } Which means.... set the parentTitle field to the "jcr:title" property of the parent resource (i.e. resource.getParent()). > > Naming nitpicks mostly, YAMF looks good to me, thanks! I need all the naming help I can get :) Thanks, Justin > -Bertrand
