Actually I nearly forgot Oleg you were driving the Google summer of code last year, what do you think?
On Tue 4 Jul 2017 at 17:44, Stephen Connolly < [email protected]> wrote: > Ulli, > > What do you think on this proposal, given that you have contact with a lot > of students who have been trying to get to grips with the Jenkins code base. > > Would it make their task easier? > > -Stephen > > On 3 July 2017 at 05:21, Stephen Connolly <[email protected] > > wrote: > >> I have been developing Jenkins plugins and changes to Jenkins core for >> more than 10 years now. As such, I have internalized a lot of the knowledge >> about how to develop against Jenkins and more specifically against Stapler. >> >> When I talk to people trying to start out development against Jenkins, >> the magic of Stapler seems to be a big source of confusion - at least to >> the people I have talked to. >> >> Prospective developers get confused: >> >> - between primary facets and facet fragments. >> - where to put facets and facet fragments. >> - which facet fragments are optional and which ones are mandatory >> >> (Did you even know that those jelly / groovy views are called facets and >> that there are two kinds?) >> >> Some of those concerns affect me too... but I am a seasoned Jenkins >> developer CMD+SHIFT+P, *, ENTER and IntelliJ is showing me a full list of >> facets and facet fragments from the class hierarchy and I can search each >> one looking for the <st:include page="fragment" optional="true"> to >> discover that there is an optional facet fragment... and then try and dig >> through the Jelly / Groovy logic to determine when and why that fragment >> should be included. >> >> It doesn't just stop there... There are those methods that we expect >> Stapler to invoke for us... typically they are the doFillXYZItems() or >> doCheckXYZ() methods. I find myself having to mark them up like: >> >> @SuppressWarnings("unused") // stapler >> public FormValidation doCheckWidgetValue(@QueryParameter String value) { >> ... >> } >> >> >> So that my IDE stops complaining about the dead code. It's not a big deal >> in the grand scheme of things, but I'd much rather have an annotation on >> the method to signify that we expect the method to be used by stapler... >> Right now I could do that using >> >> @WebMethod(name="checkWidgetValue") >> public FormValidation doCheckWidgetValue(@QueryParameter String value) { >> ... >> } >> >> >> If I don't like that, I guess I could use the newer HTTP verb annotations >> in stapler: >> >> @GET // stapler >> public FormValidation doCheckWidgetValue(@QueryParameter String value) { >> ... >> } >> >> >> But the simple @GET is not screaming out stapler to me, so I feel >> compelled to add a // stapler comment to remind myself that the >> annotation is used by stapler >> >> Then we hit the actual name that stapler exposes things on. There are >> some conventions that stapler follows... and they are magic conventions... >> so much so that I just keep http://stapler.kohsuke.org/reference.html as >> the first bookmark in my bookmark bar. >> >> I think we can do better. I think we should do better, and here is my >> proposal. >> >> We should introduce some annotations. Make them available against older >> versions of Jenkins so that we don't have to wait for everyone to update >> their plugin baseline. The annotations can be added as a plugin dependency >> in the parent pom, that everyone can use them just by upgrading the plugin >> parent pom. >> >> Here are the class annotations I think we need: >> >> - An annotation (I propose @StaplerObject) that says "This object is >> expected to bound to a URL by Stapler, the Stapler annotations are >> complete, check that there are no facets / fragments without a >> corresponding annotation and no magic on this class please" (so if there >> is, say a rename.jelly but no @StaplerFacet("rename") or >> @StaplerFragment("rename") then you get an error) >> - An annotation (I propose @StaplerFacet and the repeatable container >> @StaplerFacets) that says "Here are the facets that this object is >> expected to have" (doesn't have to be on a @StaplerObject but when >> not on a @StaplerObject we can only check for missing expected >> facets, we cannot check for "unexpected" facets - i.e. I created a facet >> with a spelling mistake or typo in the name) >> - An annotation (I propose @StaplerFragment and the repeatable >> container @StaplerFragments) that says "Here are the facet fragments >> that this object is required or may optionally have" (quite often will not >> be on a @StaplerObject) >> >> >> So I envision something like this (pre-Java 8): >> >> @StaplerObject >> @StaplerFacets({ >> @StaplerFacet("index"), >> @StaplerFacet("config") >> }) >> @StaplerFragments({ >> @StaplerFragment("main"), >> @StaplerFragment("side-panel"), >> @StaplerFragment(value="tasks",optional=true) >> }) >> public class Widget { ... } >> >> >> When compiled targeting Java 8 this would become: >> >> @StaplerObject >> @StaplerFacet("index") >> @StaplerFacet("config") >> @StaplerFragment("main") >> @StaplerFragment("side-panel") >> @StaplerFragment(value="tasks",optional=true) >> public class Widget { ... } >> >> >> There would be an annotation processor that could do some checks: >> >> - If you add @StaplerFragment(..., optional=false) then any >> non-abstract class must have that fragment somewhere in the class >> hierarchy >> - If you add @StaplerFacet then any non-abstract class must have that >> fragment somewhere in the class hierarchy >> - If you add @StaplerObject then only the named facets and facet >> fragments must be present for that class (to catch somebody calling the >> facet resource with an incorrect name - it should be side-panel2.jelly >> but they called it sidepanel2.jelly) >> >> >> Then, while we are at it, I'd also like to add method / field annotations: >> >> - An annotation (I propose @StaplerPath and the repeatable container >> @StaplerPaths) that says "here is the URL segment names that this >> method / field matches when Stapler is binding the URL". This annotation >> can be used to enable easier refactoring without risk of breaking the URLs >> because we can rename the getters and keep the URL scheme as before or >> even >> keep the legacy URLs but direct to the new URLs via the UI. >> - Annotations (I propose @StaplerHEAD / @StaplerGET / @StaplerPOST / >> @StaplerPUT / @StaplerPATCH / @StaplerDELETE plus non standard >> methods supported with @StaplerMethod("...") and then a repeatable >> container of @StaplerMethods - the repeatable container would only >> take @StaplerMethod annotations though) that cover the different HTTP >> methods (a.k.a. verbs) - we have some existing, but they need some >> extensions and I think it might be a good idea to consolidate the names, >> especially if we want to allow consumption on older baseline versions of >> Jenkins >> >> >> So this could be something like this (Pre Java 8): >> >> public class Widget { >> @StaplerPath >> public Manchu manchu; >> >> @StaplerPaths({@StaplerPath,@StaplerPath("fu")}) >> public Foo foo; >> >> @StaplerPath >> public Bar getBar() { ... } >> >> @StaplerPath(StaplerPath.DYNAMIC) >> public Object getDynamic(StaplerRequest req) { ... } >> >> @StaplerPOST >> public HttpResponse doActivate(StaplerRequest req) { ... } >> >> @StaplerPath(StaplerPath.DYNAMIC) >> @StaplerPUT >> public HttpResponse doDynamic(StaplerRequest req) { ... } >> } >> >> >> Or targeting Java 8: >> >> public class Widget { >> @StaplerPath >> public Manchu manchu; >> >> @StaplerPath >> @StaplerPath("fu") >> public Foo foo; >> >> @StaplerPath >> public Bar getBar() { ... } >> >> @StaplerPath(StaplerPath.DYNAMIC) >> public Object getDynamic(StaplerRequest req) { ... } >> >> @StaplerPOST >> public HttpResponse doActivate(StaplerRequest req) { ... } >> >> @StaplerPath(StaplerPath.DYNAMIC) >> @StaplerPUT >> public HttpResponse doDynamic(StaplerRequest req) { ... } >> } >> >> >> The annotation processor can then give additional checks: >> >> >> - @StaplerPath annotated fields must be public >> - @StaplerPath(altName) annotated fields are only allowed if >> targeting a new version Stapler that adds support for that (unless the >> name >> is the inferred name) >> - @StaplerPath annotated methods must be public and either match one >> of the allowed getter signatures or also have a HTTP method annotation and >> match the allowed action method signatures. >> - @StaplerPath(altName) annotated methods are only allowed if >> targeting a new version Stapler that adds support for that (unless the >> name >> is the inferred name) >> - @StaplerHEAD / @StaplerGET / @StaplerPOST / @StaplerPUT / >> @StaplerPATCH / @StaplerDELETE annotated methods must be public and >> match one of the allowed action method signatures. The method name must >> match the convention for action method inference (subject to >> @WebMethod(name="...") overriding when *compiling against older >> versions of Stapler* - but it would be an error to use @WebMethod if >> compiling against newer versions of Stapler because we can move to the new >> annotations in those cases) >> - If the class is @StaplerObject annotated we can do additional >> checks for overlapping facets, for example where an action method or a >> getter will hide a facet (it's ok to hide the facet for non HEAD/GET >> requests though) >> >> There are probably additional checks that we can add as we figure them >> out. >> >> >> Now there are some open questions: >> >> - Does it make sense to start all these annotations with Stapler? My >> initial stab at this was to use @Staple in place of @StaplerPath and >> to re-use the @GET / @POST / @PUT / @DELETE verbs but mandating the >> @Staple annotation so that you would have: >> >> @Staple @POST >> >> public HttpResponse doActivate(StaplerRequest req) { ... } >> >> >> rather than >> >> @StaplerPOST >> >> public HttpResponse doActivate(StaplerRequest req) { ... } >> >> >> One of the issues I found with that is class conflicts when back-porting >> the @GET / @POST / etc annotations, so we could only use those if we >> were prepared to mark 2.7 as the oldest core that could use the >> annotations, whereas we can make new annotations in a separate package >> available as far back as Stapler 1.180 == Jenkins 1.455 (or worst case >> Stapler 1.237 == Jenkins 1.651) >> >> Now we could introduce new annotations without the @Stapler prefix, but >> then your code complete would confuse you between >> @org.kohsuke.stapler.annotations.GET and @org.kohsuke.stapler.verbs.GET >> plus we still need something to give a hint that the annotation is implying >> Stapler's involvement (at least from my perspective) >> >> >> - I had thought about saying that the class level @StaplerObject >> should be enough of a flag that you should expect the other annotations... >> but the class annotation will be inherited, so you may not see it until >> you >> look at the super-class... and @Path will conflict with >> java.nio.file.Path... that might work with @Staple (and @Staples as >> the container) but while cute they do not scream to the new developers >> that >> these are involved in url binding. >> >> NOTE: In this context I see this change as being entirely opt-in. Plugin >> developers should not have to go adding the annotations - though we would >> probably apply them in Jenkins core to assist new developers. >> >> >> - If we go with the Stapler prefix, would it make sense to >> consolidate all the annotations with that prefix? >> >> - I am not convinced for the case of @ExportedBean and @Exported, >> these reflect a different cross-cutting concern and as such the pair >> seem >> named well from my PoV >> - The @JavaScriptMethod / @WithWellKnownURL annotations might be >> worthy of consolidation if we can determine better names. >> - The parameter binding annotations such as @AncestorInPath, >> @Header, @QueryParameter and @InjectedParameter I think are fine >> where they are as they will only be on an action method which already >> has >> the @StaplerPOST etc indicator. >> - The meta-annotation @InterceptorAnnotation should not be >> consolidated, in any case we could not consolidate it and retain usage >> against older cores >> - The other annotations are likely rarely used, but if we can find >> good names and people think they are generically useful rather than >> single >> use-case hacks added into the stapler API, I think consolidation might >> make >> the features more widely used. >> >> So, over to you the community of Jenkins developers: >> >> 1. Would this change have helped you get started quicker? >> 2. Would this change help you even now? >> 3. How ugly are my proposed annotation names? Can you provide a less >> ugly scheme of names? >> 4. Anything else? >> >> Stephen >> > > -- Sent from my phone -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMy8RJknVTa1cyPOS9tNGJO%2B2SccA1xSWea5_tKsmOWpEg%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
