> Am 05.07.2017 um 11:27 schrieb Stephen Connolly > <[email protected]>: > > +jenkins-dev
Uups, sorry… > > On 5 July 2017 at 02:13, Ullrich Hafner <[email protected] > <mailto:[email protected]>> wrote: > >> Am 04.07.2017 um 18:44 schrieb Stephen Connolly >> <[email protected] <mailto:[email protected]>>: >> >> 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? >> > > Up to now most students work in the area of testing (ATH), so this will not > help them (they would love to see IDs for all entry fields, but that is a > totally different topic). > > So one aspect where this could help is in knowing what to test. > > The annotations can be used to help identify the intent of the plugin author > / core and we could perhaps even develop tooling to identify ATH coverage > > >> -Stephen >> >> On 3 July 2017 at 05:21, Stephen Connolly <[email protected] >> <mailto:[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 >> > > This indeed is one of the most confusing things when developing plug-ins. It > cost a lot of time to get these things right (mostly by copying and pasting > code from another plugin). > > I'm not sure if annotations are the correct way of dealing with these > problems. I think you then need to know where to put an annotation and where > not, this will also result in searching for other plugins that do it right. > > Yes, I would also propose improving the stapler documentation in conjunction > with these changes so that it should be easier to find the "recommended" > patterns to follow. > > Though how much energy I personally have to push documentation improvements > is another question entirely ;-) Well, this is always a problem ;-) But we already have some good examples in the wiki so the documentation of these new concepts might grow in the same way by some motivated plugin authors ... > > > >> (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 do these annotations help to get rid of the unused code warnings? I think > you still need to add a @SuppressWarnings. > > Well you can configure IDEs to say "this annotation is used" and we can even > have the Stapler plugin for IntelliJ pre-register the annotations so that > IntelliJ users (I'm selfish ;-) ) don't even need to add the setting That would be very helpful! (Isn’t everybody using IntelliJ to develop plugins?;-) Maybe improving the Stapler plug-in would be a good candidate for a bachelor/master thesis… (Code completion in jelly views would be awesome) > > >> 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 >> <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. >> > > > Would there be some checks, that check for the correct path for the > corresponding index.jelly, config.jelly, global.jelly and so on? > > Yes, the idea is that when you annotate a non-abstract class with > @StaplerFacet we would check you have the facet in your class hierarchy and > error out. > > If you annotated your non-abstract class with @StaplerObject then we can go > one further and error out if you have an unexpected .jelly or .groovy file > which gives you even more help to avoid creating resources in the wrong place. > > Because of taglibs, and semi-static resources we cannot scan the classpath > for .jelly and .groovy files and assume that they are either facets or > fragments which is why the "is a complete set" checks require @StaplerObject > > >> 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: >> Would this change have helped you get started quicker? >> Would this change help you even now? > > I’m not sure if it would help to get started quicker. I think the UI mapping > in Jenkins is still (too?) complex even with these annotations. Maybe it > would make sense if you can show for an example plug-in class (and jelly > file) how it will look after using the annotations? > > Yes, my next step is to mock up the proposed annotations and apply to one or > two sample plugins as a (do not merge) PR. > > Do you have any plugins or plugin classes you would like to suggest as good > examples to trial? My plugins are typically too complex as a starting point for beginners (due to the analysis-core dependency and the number of implemented extension points), so it might be better to start with a simpler one. > > >> How ugly are my proposed annotation names? Can you provide a less ugly >> scheme of names? > > The names are fine. > > Thanks, one of the concerns I have is that we need some rough consensus on > the names as they are liable to become a bikeshed painting exercise >> Anything else? >> Stephen -- 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/8B749EE4-C9A9-4B65-B4E9-5369EA3282EF%40gmail.com. For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: Message signed with OpenPGP
