Hi, Am Freitag, den 26.10.2007, 15:28 +0200 schrieb Bertrand Delacretaz: > 1) The "helpers" package name > Carsten mentions that in the other thread - in microsling the idea is > to use "helpers" as the name of all packages which contain "boring" > stuff, classes that you don't need to study to understand the API. > > I like that name, as it's a clear flag for people not to waste time > looking at that stuff. > > 2) ProcessTracker > The javadoc comment says "tracks the progress of request processing", > so I'd call that "RequestProgressTracker" instead, "process" is too > generic IMHO.
Agreed. > > 3) Service-related classes (ServiceLocator + exception) > I'd put these in a "services" package, they're more than boring helpers. Agreed. > > 4) params package > It might make more sense to call this package "request", and include > RequestPathInfo in there. Agreed (and Carsten also mentioned this). > > 5) MetaData > I'd call this ResourceMetadata instead, MetaData is too generic, and > "metadata" is one word so I don't like the CamelCaseDInTheMiddle too > much. Well, I though MetaData is more generic (and I see it as two words). But I agree, we should probably call it ResourceMetadata. > > 6) SlingScript > Should we use getResource() instead of getScriptPath()? In our model, > a Script is always created from a Resource...not sure about that. I don't think, that a script is always a resource. It might also be an OSGi service registered with the given path. In this case of course the "path" is not actually the absolute correct word. I would not change this to a Resource in this use case. > > I'd use getScriptReader() instead of getScript() as the method name. Agreed. > > 7) SlingScriptEngine > > Should we have two eval() methods? > > eval(script, props, request, response) for servlet environments > > eval(script, props, reader, writer) for "internal" scripts I think eval(Script, Map<String, Object>) suffices it and should encompass enough information needed. > > 8) HttpConstants > As we're requiring java 5, we can make this a class instead of an > interface, and use it with "import static" to avoid the Constant > Interface Antipattern. Agreed > > 9) Constants > Same as HttpConstants, make that a class, and I'd rename that class to > SlingConstants - there are too many Constants class out there I guess. Agreed. BTW: The Constants interface is not implemented in any part of Sling (AFAIK) exactly for this reason. But making it a class, is also ok. > > I'd also move that class to the "helpers" package - it is not > interesting to study. I do not actually agree. But it is just sort of a guts feeling :-) > > 10) SlingHttpServlet... > I'd move these classes to the "servlets" package, and maybe move the > SlingXMethodsServlets to servlets/helpers - that's what they are I > think, and that would prevent the package from having too much stuff. I do not agree as the SlingHttpServletRequest/Response interfaces are two of the central interfaces of interest to Sling programmers: These are the first interfaces they are confronted with, so I would rather keep them upfront. > > 11) SlingScriptResolver > As mentioned elsewhere, provide an additional method to resolve a > script by path, or maybe > > resolveScript(String path,String [] pathsWhereToSearchForScripts); Agreed with exception: I think the search path thing is either an implementation specific functionality or may be described by a getter (don't like setter in interfaces :-) ). I would not add this parameter to the signature of the resolveScript method. > > 12) javadocs > In general I find the javadoc comments too verbose for my taste. But > we can talk about that once the naming and packages are in place, > maybe I'll try doing a patch to make the docs as terse as possible, > without losing information, and we can decide whether we want that. Well, I disagree :-) The API speficies a central part of Sling and as such should be specific as much as possible to not run the danger of being underspecified and thus create problems in the future, where people might expect something, which does not match the implementation. > Ouch...lots of comments, but that's mostly naming and package > nitpicking, the actual API looks good! Thanks, but as the changes are big, comments will be big :-) Regards Felix
