-
Notifications
You must be signed in to change notification settings - Fork 102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make ClassDescriptor deterministic #117
Make ClassDescriptor deterministic #117
Conversation
…thods, particularly overloads.
public HttpResponse doX(StaplerRequest req, StaplerResponse rsp) { | ||
return HttpResponses.plainText("doX(StaplerRequest, StaplerResponse)"); | ||
} | ||
@WebMethod(name = "x") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means that a magically named method will have precedence over a @WebMethod
? This seems counterintuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then deprecate your other overloads. Again, the exact ordering of (nondeprecated) methods is arbitrary, so long as it is predictable so any field errors can be reproduced by developers. If you have more than one, you are in the wrong.
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. |
+0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. 🐝
@reviewbybees done |
Has sometimes been observed that a web method with multiple overloads will randomly run one or the other (according to JVM implementation details), which is confusing and can lead to bugs, especially if all but one overload was intentionally deprecated. This patch simply picks one nondeprecated overload consistently so there are no surprises (as in junit-team/junit4#293 for example).
@reviewbybees