Hi, Thanks Felix for the summary of changes.
Overall I think this is very close to the microsling API, so if it allows Sling OSGi to work as well as it did before that's a great thing! Here are my comments on the sling-api revision 588629 (hang on, lots of them ;-) 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. 3) Service-related classes (ServiceLocator + exception) I'd put these in a "services" package, they're more than boring helpers. 4) params package It might make more sense to call this package "request", and include RequestPathInfo in there. 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. 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'd use getScriptReader() instead of getScript() as the method name. 7) SlingScriptEngine Should we have two eval() methods? eval(script, props, request, response) for servlet environments eval(script, props, reader, writer) for "internal" scripts 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. 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. I'd also move that class to the "helpers" package - it is not interesting to study. 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. 11) SlingScriptResolver As mentioned elsewhere, provide an additional method to resolve a script by path, or maybe resolveScript(String path,String [] pathsWhereToSearchForScripts); 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. Ouch...lots of comments, but that's mostly naming and package nitpicking, the actual API looks good! -Bertrand
