LinearScanner cannot be called with multiple heads

[jglick]

Would make mistakes like in jenkinsci/workflow-support-plugin#22 less likely.

@reviewbybees esp. @svanoort

[ghost]

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

[svanoort]

Why so many overrides for LinearScanner? It should be sufficient to just override the setup method since everything delegates to that.

[jglick]

The only real override is of setHeads. All the rest are just to adjust Javadoc and (where possible) add a deprecation warning.

[svanoort]

@jglick That just adds noise and clutter -- it's enough to mention this in the top-level javadoc, since that's what defines the behavior for each FlowScanner.

[svanoort]

To be clear: the change itself seems reasonable and prudent.

[jglick]

it's enough to mention this in the top-level javadoc

Assuming someone reads the top-level Javadoc carefully. By deprecating the particular methods which are advertised to take a collection of nodes—but which really will not work if more than one is passed—the limitation becomes much more apparent. For example, the code I wrote in jenkinsci/workflow-support-plugin#19 would have shown a deprecation warning in my IDE.

[svanoort]

Assuming someone reads the top-level Javadoc carefully.

Well, if you expect people won't read the javadoc, why both making any change to it? ;)

I am -1 on deprecating these methods: it's perfectly legit to throw an exception here but the reason they will accept a list is so that you can pass in getCurrentHeads directly.

[jglick]

if you expect people won't read the javadoc, why both making any change to it?

Deprecation warnings appear more proactively. You do not need to go looking at the Javadoc to see that there is a problem.

the reason they will accept a list is so that you can pass in getCurrentHeads directly

And that is exactly what you should not do, because then your code will work fine until the day something uses parallel and then it will explode.

[jglick]

IOW with the deprecations in place, the caller is forced to pay attention to the fact that the scanner cannot process multiple heads, and so you must think about what you want to do instead. In my case, it was to scan from each head in sequence.

[jglick]

CI build passed, status notifications seems to be down.

[svanoort]

Looking at this again, I was uncertain about it before, but now think it's flat-out not a good idea to take this path: we're making LinearScanner violate the contracts of AbstractFlowScanner in a surprising way by throwing an IllegalArgumentException with legal input.

Logging a warning with multiple heads is appropriate and would assist debugging. Let's do that instead -- it won't cause a runtime failure, but will still provide guidance.

[jglick]

we're making LinearScanner violate the contracts of AbstractFlowScanner

Yes, that is exactly the idea. The behavior when multiple heads are passed already violates any sensible expectations; a clear exception makes it no worse.

Logging a warning with multiple heads is appropriate and would assist debugging.

Somewhat, but only after you have made the mistake. In the case I encountered, that would have made developing the fix slightly faster, but would not have stopped me from producing the buggy release to begin with.

If you are talking about retaining the deprecations but switching the two IllegalArgumentExceptions to warnings, I still think that is less desirable than the current PR, but certainly better than nothing.

[svanoort]

Not 100% thrilled with throwing in the useless TODOs for deprecation warnings (why add a TODO if the task is a few minutes?) but it's generally acceptable.

[jglick]

useless TODOs for deprecation warnings

Was not immediately clear to me what the replacement for those methods would be. New overloads in AbstractFlowScanner? Or non-overriding methods in LinearScanner? Or no replacement (caller expected to use different idioms)?

[svanoort]

@jglick Overloads would be my suggestion in AbstractFlowScanner, or in the Linear ones (either is fine by me)

[jglick]

Makes sense. Deferring that until a rainy day (or someone needs such calls).