|
In https://github.com/jenkinsci/workflow-plugin/commit/e24a64a7a6404be97f42340f4c44d3988be1052c you added
createBodyBlockNode = (name==null);
Is this not backwards? The Javadoc says that if name == null, the body is "anonymous", and
anonymous body invocation suppress the generation of BlockStartNode/BlockEndNode
which makes sense. Yet here you are saying that an anonymous body should create a body block node, which looks like a mistake. There appears to be no call to this method passing an actual display name anywhere in the codebase, and no test of how it should behave. I suspect that a call to non-null displayName would actually produce an IllegalStateException in start, since then startNodeActions would have a LabelAction. Note that ParallelStepExecution does call withStartAction, the only one to do so, and effectively sets a display name for the block, yet not using regular LabelAction; it seems the only purpose for the custom subclass is to assert branch names in a test.
Furthermore, displayName is null by default, yet the Javadoc explicitly suggests calling withDisplayName(null), which will have no effect since createBodyBlockNode is also true by default. And many—but not all—steps indeed call withDisplayName(null).
This all seems confused. Either blocks should be anonymous by default, in which case createBodyBlockNode should default to false, and all the calls to withDisplayName(null) should be deleted, and the name parameter should be marked @Nonnull, and the Javadoc should confirm the default behavior, and ParallelStepExecution should be handled somehow; or blocks should be visible by default, in which case displayName should have some default value (perhaps computed according to StepDescriptor.displayName?) and you would need to call withDisplayName(null) to turn it off. The first option seems more appropriate to me.
|
