IntelliJ/Java: Duplicate code -- Refactored as UnstableAwareCommandInterpreter.helpCheckUnstableReturn #4077
Conversation
…terpreter.helpCheckUnstableReturn
| } | ||
|
|
||
| public boolean isApplicable(Class<? extends AbstractProject> jobType) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| static String invalidExitCodeZero() { |
There was a problem hiding this comment.
Need to remove this strategy as it won't do what I want, per #4034 (comment)
| super(command); | ||
| } | ||
|
|
||
| static String invalidExitCodeZero() { return null; }; |
There was a problem hiding this comment.
The problem is that the caller is:
public static final class DescriptorImpl extends BuildStepDescriptor<Builder> {
...
public FormValidation doCheckUnstableReturn(@QueryParameter String value) {
return helpCheckUnstableReturn(value);
I'm not sure how to get past this. I guess the way to do it is to pass an object to helpCheckUnstableReturn
| if (unstableReturn == 0) { | ||
| return FormValidation.warning(invalidExitCodeZero()); | ||
| } | ||
| if (unstableReturn < 1 || unstableReturn > 255) { |
There was a problem hiding this comment.
The code in BatchFile uses Integer.MIN_VALUE and Integer.MAX_VALUE so this would be changing the behaviour.
| return FormValidation.error(hudson.tasks.Messages.Shell_invalid_exit_code_range(unstableReturn)); | ||
| } | ||
| return FormValidation.ok(); | ||
| public static FormValidation doCheckUnstableReturn(@QueryParameter String value) { |
There was a problem hiding this comment.
I'm not sure making it public is safe and backward compatible.
| return FormValidation.error(hudson.tasks.Messages.BatchFile_invalid_exit_code_range(unstableReturn)); | ||
| } | ||
| return FormValidation.ok(); | ||
| static public FormValidation doCheckUnstableReturn(@QueryParameter String value) { |
There was a problem hiding this comment.
Why are we investing time in improving a method that shouldn't be used?:
@Restricted(DoNotUse.class)
This type, field, or method shall never be referenced from anywhere at all.
There was a problem hiding this comment.
FTR the @Restricted(DoNotUse.class) is meant for Jelly calls. It's a method that is called by Stapler, detected using reflection during the form build process.
There was a problem hiding this comment.
Are you sure? I think it's used to warn programmers
There was a problem hiding this comment.
daniel-beck
added
the
plugin-api-changes
|
Although the improvements are good, my feeling is that we don't need to improve it because we are breaking the API and the methods shouldn't be used. So my proposal is to close this PR unless the API changes are reverted. Anyway, thanks Jsoref for your work. |
rsandell
added
the
proposed-for-close
|
I'm closing this given there's been no activity for some time. Please feel free to reopen if you plan to work on this again. |
This is for discussion.
Notes:
Proposed changelog entries
Internal:Internal Java code cleanupSubmitter checklist
* Use the
Internal:prefix if the change has no user-visible impact (API, test frameworks, etc.)Desired reviewers
@Wadeck