Skip to content
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

IntelliJ/Java: Duplicate code -- Refactored as ExtensionFinder.getClassFromIndex #4079

Merged

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Jun 18, 2019

This is for discussion.

Notes:

  • Commits was probably created by IntelliJ for Java cleanup #4034
  • In general, commits should not change program behavior at all

Proposed changelog entries

  • Internal: Internal Java code cleanup

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@Wadeck

@jsoref jsoref mentioned this pull request Jun 18, 2019
4 tasks
Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for the split, lot easier to review the changes!

nitpick: as you modified the duplicate code, you can ensure consistency by putting { ... } around the last else statement (the one for throw new AssertionError();) (looking forward to have a better multiline suggestion feature from GH)

@jsoref jsoref force-pushed the extensionfinder-getclassfromindex branch from 14bd9f2 to 4e1fe91 Compare June 19, 2019 15:04
@jsoref jsoref force-pushed the extensionfinder-getclassfromindex branch from 4e1fe91 to e95fe1c Compare June 19, 2019 15:06
@jsoref
Copy link
Contributor Author

jsoref commented Jun 19, 2019

@Wadeck: added as a distinct commit because there are two places where that seems useful.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a protected method in the public class, so formally it is a part of binary API. Please either add Javadoc (with @since TODO) or make it private. The rest looks good to me

@jsoref jsoref force-pushed the extensionfinder-getclassfromindex branch from e95fe1c to faff454 Compare June 27, 2019 18:56
@@ -758,5 +737,20 @@ private Level logLevel(IndexItem<Extension, Object> item) {
}
}

private static Class<?> getClassFromIndex(IndexItem<Extension, Object> item) throws InstantiationException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oleg-nenashev: marked as private

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@oleg-nenashev oleg-nenashev merged commit 1165201 into jenkinsci:master Jun 28, 2019
@jsoref jsoref deleted the extensionfinder-getclassfromindex branch June 28, 2019 20:07
jsoref added a commit to jsoref/jenkins that referenced this pull request Jul 3, 2019
…ssFromIndex (jenkinsci#4079)

* IntelliJ/Java: Duplicate code -- Refactored as ExtensionFinder.getClassFromIndex

* Adding braces around throw new AssertionError
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants