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

Document and refactor the CanonicalIdResolver extension point #3140

Merged
merged 2 commits into from Dec 3, 2017

Conversation

3 participants
@oleg-nenashev
Copy link
Member

oleg-nenashev commented Nov 14, 2017

The extension point has been introduced by @ndeloof in af42534
It does not follow common extension point practices (e.g. “all()” methods) and has no Javadoc.

Proposed changelog entries

No functional code changes, so no JIRA ticket/tests. I can create them on-demand

Desired reviewers

@reviewbybees @ndeloof

Document and refactor the CanonicalIdResolver extension point.
The extension point has been introduced in af42534
It does not follow common extension point practices (e.g. “all()” methods) and has no Javadoc.

So I decided to polish it a bit. The core also adds some TODOs for review
@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Nov 14, 2017

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.

@@ -401,23 +401,16 @@ public synchronized void doSubmitDescription( StaplerRequest req, StaplerRespons
* An existing or created user. May be {@code null} if a user does not exist and
* {@code create} is false.
*/
public static @Nullable User get(String idOrFullName, boolean create, Map context) {
public static @Nullable User get(String idOrFullName, boolean create, @Nonnull Map context) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 14, 2017

Author Member

I cannot find usages of this parameter at all, but I decided to be on the safe side

@oleg-nenashev oleg-nenashev requested a review from Wadeck Nov 14, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Dec 1, 2017

@ndeloof

ndeloof approved these changes Dec 1, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Dec 1, 2017

Thanks @ndeloof! Just doing some polishing while I am around

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Dec 1, 2017

@oleg-nenashev oleg-nenashev merged commit e1d175c into jenkinsci:master Dec 3, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment