onLoad(FlowExecutionOwner) is meant to throw IOException

[jglick]

Reverting an incorrect change (I suppose caused by unvalidated use of some refactoring tool by @offa) from #185.

[dwnusbaum]

I think FlowExecutionOwner.getListener also needs to have throws IOException added back for WorkflowRun.Owner.getListener.

[jglick]

I think FlowExecutionOwner.getListener also needs to have throws IOException added back

workflow-api-plugin/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionOwner.java

Line 128 in 7f5f175

public @NonNull TaskListener getListener() throws IOException {

is still OK.

workflow-api-plugin/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionOwner.java

Line 179 in 7f5f175

@Override public TaskListener getListener() {

is just a private impl, should not be a problem.

[jglick]

@offa please disable whichever IDE suggestion you may have to automatically strip unthrown exceptions as this is not safe to use without human inspection in general.

[basil]

@offa please disable whichever IDE suggestion you may have to automatically strip unthrown exceptions as this is not safe to use without human inspection in general.

Yeah, I tend to ignore this IDE suggestion in general. The one case where I have found it useful is for removing e.g. throws Exception from test methods that don't need it, but even in that case the value of the suggestion seems dubious.

[offa]

Sorry for introducing a regression with my PR; I've checked changes, ran tests etc. but didn't verify downstream consumers like cps.

In my personal opinion declared but not thrown exceptions should get removed as they lie to the caller, may reduce readability and force them to write additional code to handle situations that wont ever appear or are impossible. Without breaking anything of course – unlike here.

Again, my opinion and it's totally fine to disagree.

If there's a consensus or recommendation to leave not-thrown exceptions alone I'm fully ok with that.

Thanks @jglick for fixing it up.

[jglick]

Sorry for introducing a regression with my PR

No worries, everyone does sometimes! This only caused a source incompatibiilty (no effect on Jenkins users updating).

declared but not thrown exceptions should get removed

In some cases sure. But not if the method is intended to be overridden outside the module. Or if it is a test which will quite possibly sooner or later wind up throwing a checked exception. Or if the method is part of some public API and there is the notion to later change its body to something that might throw that exception. Or if there are some callers (in another module) which would no longer compile because they currently catch this exception, unless you have a plan to fix them up too. And so on. Basically it requires human review in each case, so is not safe to enable in batch as part of mechanical code cleanup PRs, and in most cases I have seen I would actually prefer the change be reverted even if it does not much matter either way.