Yes I was thinking that a custom javadoc handler would be useful. Especially as there are inheritance of facets and fragments that we'd want to see documented correctly.
On 7 July 2017 at 02:37, Robert Sandell <[email protected]> wrote: > I agree with Stephen here; adding methods seems risky and could break > existing plugins, at least in a backwards compat sense. > > But I at the same time do like the idea of getting javadoc for the facets > and fragments. So can we get both somehow? > E.g. if there is a @StaplerFacet("login") or @StaplerFragment("main") there > should be a corresponding > > @facet login the login page. > @fragment main add this to change the main part of the configuration page. > > in the javadoc of the class? > > /B > > 2017-07-06 13:16 GMT+02:00 Stephen Connolly <stephen.alan.connolly@gmail. > com>: > >> >> >> On 5 July 2017 at 18:10, Kohsuke Kawaguchi <[email protected]> wrote: >> >>> FWIW, I've already discussed this offline with Stephen and I'm >>> generally +1. >>> >>> One suggestion I had for him is to consider using a Java method >>> definition instead of @StaplerFacet and @StaplerFragment. That is, instead >>> of this: >>> >>> @StaplerFacets({ >>> @StaplerFacet("index"), >>> @StaplerFacet("config") >>> }) >>> @StaplerFragments({ >>> @StaplerFragment("main"), >>> @StaplerFragment("side-panel"), >>> @StaplerFragment(value="tasks",optional=true) >>> }) >>> public class Widget { ... } >>> >>> Consider that: >>> >>> public class Widget { >>> @StaplerFacet protected void index() {} >>> @StaplerFacet protected void config() {} >>> >>> @StaplerFragment protected abstract void main(); >>> @StaplerFragment("side-panel") protected abstract void sidepanel(); >>> @StaplerFragment protected void tasks(); >>> } >>> >>> >> Well at least this is a better explanation than you gave me verbally ;-) >> >> >>> I liked this better because ... >>> >>> - It fits with the original design thinking behind Stapler, which is >>> that views are like methods. Their inheritance and override semantics are >>> all modeled after methods. >>> - It avoids a long list of @StaplerFragment/@StaplerFacet at the top >>> of the class declaration. We have some objects in the core that has >>> quite a >>> few views. >>> >>> So let's see: >> >> $ ( cd core/src/main/resources ; find . -name \*.jelly -exec dirname {} >> \; ; find . -name \*.groovy -exec dirname {} \; ) | sort | uniq -c | grep >> -v ./lib/ | sort -n | tail -n 10 >> 7 ./hudson/model/AbstractProject >> 7 ./hudson/slaves/SlaveComputer >> 7 ./jenkins/install/SetupWizard >> 9 ./hudson/model/Run >> 11 ./hudson/PluginManager >> 11 ./hudson/security/HudsonPrivateSecurityRealm >> 12 ./hudson/model/Job >> 13 ./hudson/model/View >> 14 ./hudson/model/Computer >> 25 ./jenkins/model/Jenkins >> >> So there are not altogether too many classes in Jenkins having lots of >> fragments / facets >> >> I think an argument based on a couple of really big classes like Jenkins >> having 25 lines of >> >> @StaplerObject >> @StaplerFacet("_restart") >> @StaplerFacet("_safeRestart") >> @StaplerFacet("_script") >> @StaplerFacet("_scriptText") >> @StaplerFacet("accessDenied") >> @StaplerFacet("configure") >> @StaplerFacet("configureExecutors") >> @StaplerFacet("fingerprintCheck") >> @StaplerFacet("legend") >> @StaplerFacet("load-statistics") >> @StaplerFacet("login") >> @StaplerFacet("loginError") >> @StaplerFacet("manag_") >> @StaplerFacet("manage") >> @StaplerFacet("newView") >> @StaplerFacet("noPrincipal") >> @StaplerFacet("oops") >> @StaplerFacet("opensearch.xml") >> @StaplerFacet("projectRelationship-help") >> @StaplerFacet("projectRelationship") >> @StaplerFacet("systemInfo") >> @StaplerFacet("threadDump") >> @StaplerFragment("_api") >> @StaplerFragment("downgrade") >> @StaplerFragment("sidepanel") >> public class Jenkins { >> ... >> } >> >> compared with >> >> @StaplerObject >> public class Jenkins { >> @StaplerFacet protected void _restart() {} >> @StaplerFacet protected void _safeRestart() {} >> @StaplerFacet protected void _script() {} >> @StaplerFacet protected void _scriptText() {} >> @StaplerFacet protected void accessDenied() {} >> @StaplerFacet protected void configure() {} >> @StaplerFacet protected void configureExecutors() {} >> @StaplerFacet protected void fingerprintCheck() {} >> @StaplerFacet protected void legend() {} >> @StaplerFacet @StaplerPath("load-statistics") protected void >> loadStatistics() {} >> @StaplerFacet protected void login() {} >> @StaplerFacet protected void loginError() {} >> @StaplerFacet protected void manag_() {} >> @StaplerFacet protected void manage() {} >> @StaplerFacet protected void newView() {} >> @StaplerFacet protected void noPrincipal() {} >> @StaplerFacet protected void oops() {} >> @StaplerFacet @StaplerPath("opensearch.xml") protected void >> opensearchXml() {} >> @StaplerFacet @StaplerPath("projectRelationship-help") protected void >> projectRelationshipHelp() >> @StaplerFacet protected void projectRelationship() {} >> @StaplerFacet protected void systemInfo() {} >> @StaplerFacet protected void threadDump() {} >> @StaplerFragment protected void _api() {} >> @StaplerFragment protected void downgrade() {} >> @StaplerFragment protected void sidepanel() {} >> ... >> } >> >> just to get the javadoc comments and leverage abstract and final... I >> really do not like the extra effort of typing in all those method names... >> >> Perhaps the biggest issue for me is that to retrofit that into Jenkins we >> would need to take great care that these methods we are adding as facet and >> fragment markers do not have names that may potentially conflict with >> subclasses... >> >> If we add >> >> @StaplerObject >> public abstract class Descriptor ... { >> ... >> @StaplerFragment protected abstract void config(); // doesn't really >> matter if abstract or not here >> ... >> } >> >> we may make it impossible for a concrete subclass to have a >> >> public MyDescriptor extends Descriptor ... { >> >> private MyConfig config; >> >> ... >> >> public MyConfig config() { >> return config.clone(); >> } >> >> } >> >> as they will be unable to retain binary compatibility with their existing >> "config" method when they upgrade... >> >> So that means we will need to do >> >> @StaplerObject >> public abstract class Descriptor ... { >> ... >> @StaplerFragment @StaplerPath("config") protected abstract void >> _retrofit_config(); >> ... >> } >> >> Thinking some more, I see issues with plugin authors being thereby forced >> to add implementations when they upgrade core... ok so they are quick and >> the IDE will help... but having to add all those >> >> public MyDescriptor extends Descriptor ... { >> >> private MyConfig config; >> >> ... >> >> public MyConfig config() { >> return config.clone(); >> } >> >> @StaplerFragment @StaplerPath("config") protected void >> _retrofit_config() {} >> } >> >> All the while hoping that the IDE has copied the method annotations >> (otherwise the method is not annotated) and the plugin author has not opted >> into @StaplerObject yet >> >> That, to me, does not make for a compelling developer experience. >> >> WDYT? >> >> I am inclined to reject your method based idea and stick with class level >> annotations... there are only 11 classes in all of Jenkins core with more >> than 5 facets / fragments >> >> >>> - It creates a nice to place to describe view as javadoc. >>> - 'abstract' and 'final' provides convenient semantics and javac >>> does that work for us. One less thing to do for our annotation processor >>> >>> Obvious downside is that those are not real methods. >>> >>> On Mon, Jul 3, 2017 at 5:21 AM 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 >>>> >>>> -- >>>> 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/ms >>>> gid/jenkinsci-dev/CA%2BnPnMz-m49TK7Em%2BxBNb%2BV98dBCz9CrrPX >>>> g3uW6%2B_x3KX5gOQ%40mail.gmail.com >>>> <https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMz-m49TK7Em%2BxBNb%2BV98dBCz9CrrPXg3uW6%2B_x3KX5gOQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>> . >>>> For more options, visit https://groups.google.com/d/optout. >>>> >>> -- >>> Kohsuke Kawaguchi >>> >>> -- >>> 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/ms >>> gid/jenkinsci-dev/CAN4CQ4wDS34DE-PQVK6tsCQD5AXWcxCMEVkCgh5MN >>> ymRNVcLPw%40mail.gmail.com >>> <https://groups.google.com/d/msgid/jenkinsci-dev/CAN4CQ4wDS34DE-PQVK6tsCQD5AXWcxCMEVkCgh5MNymRNVcLPw%40mail.gmail.com?utm_medium=email&utm_source=footer> >>> . >>> >>> For more options, visit https://groups.google.com/d/optout. >>> >> >> -- >> 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/ms >> gid/jenkinsci-dev/CA%2BnPnMwio9PeC3uhC_i4XqQ_7OY2dN2Hx34e6hS >> mHVAeG7RiRA%40mail.gmail.com >> <https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMwio9PeC3uhC_i4XqQ_7OY2dN2Hx34e6hSmHVAeG7RiRA%40mail.gmail.com?utm_medium=email&utm_source=footer> >> . >> >> For more options, visit https://groups.google.com/d/optout. >> > > > > -- > Robert Sandell > *Software Engineer* > *CloudBees Inc.* > > -- > 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/CALzHZS10%2BzMD58BLJBiksxY7ch7sDQ7rPQEN9 > y-SJACGhREoTA%40mail.gmail.com > <https://groups.google.com/d/msgid/jenkinsci-dev/CALzHZS10%2BzMD58BLJBiksxY7ch7sDQ7rPQEN9y-SJACGhREoTA%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/d/optout. > -- 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%2BnPnMx7mkF%2BbjcW2%3D0KiwN9GDimNEsSzMaf3ctDRsUhB2_v8Q%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
