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.

Sign up for GitHub

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

Jump to bottom

Java cleanup #3929

Merged
merged 11 commits into from
Apr 1, 2019
Merged

Java cleanup #3929

merged 11 commits into from
Apr 1, 2019

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Mar 8, 2019 •
edited by batmat

This is for discussion.

Notes:

  • Most commits were created by IntelliJ (the descriptions are mine because I lost the details)
  • Individual commits are mostly self contained
  • In general, most 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

jvz
jvz approved these changes Mar 11, 2019
View reviewed changes
Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

Looks good overall. Have a couple suggestions.

core/src/main/java/hudson/Main.java Show resolved Hide resolved
core/src/main/java/hudson/model/Computer.java Outdated Show resolved Hide resolved
core/src/main/java/hudson/model/Fingerprint.java
List<FingerprintFacet> r = new ArrayList<FingerprintFacet>(getFacets());
Collections.sort(r,new Comparator<FingerprintFacet>() {
List<FingerprintFacet> r = new ArrayList<>(getFacets());
r.sort(new Comparator<FingerprintFacet>() {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like you can likely use Comparator.comparingLong(FingerprintFacet::getTimestamp) instead of the manually written one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like that happens often in jenkins, I might go back and add a commit later which does that cleanup:

jenkins/test/src/test/java/jenkins/model/ExtensionTypeHierarchyTest.java

Line 55 in 22aa2e6

return o1.getClass().getName().compareTo(o2.getClass().getName());

jenkins/core/src/main/java/hudson/views/MyViewsTabBar.java

Line 89 in 0795e89

return o1.getDisplayName().compareTo(o2.getDisplayName());

jenkins/test/src/test/java/hudson/model/UsageStatisticsTest.java

Line 158 in d9aa128

return j1.getString("name").compareTo(j2.getString("name"));

https://stackoverflow.com/questions/32995559/reverse-a-comparator-in-java-8 suggests Comparator.reverseOrder() might be able to handle:

jenkins/core/src/main/java/hudson/model/RunMap.java

Line 159 in 22aa2e6

return -o1.compareTo(o2);

Copy link
Member

Choose a reason for hiding this comment

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

Figures because this feature was added in Java 8, and you're the main person making Java 8 related updated to Jenkins at the moment. ;)

core/src/main/java/hudson/model/Queue.java Outdated Show resolved Hide resolved
core/src/main/java/hudson/model/TaskThread.java Outdated Show resolved Hide resolved
@jsoref jsoref force-pushed the java-cleanup branch 2 times, most recently from 78fc57d to 886e350 Compare March 15, 2019 18:19
varyvol
varyvol reviewed Mar 25, 2019
View reviewed changes
Copy link

@varyvol varyvol left a comment

Choose a reason for hiding this comment

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

Really hard to review such amount of changes. In my opinion you should split this kind of PR into several ones.

core/src/main/java/jenkins/model/Jenkins.java
@@ -3403,9 +3403,6 @@ public void onAttained(Milestone milestone) {
LOGGER.log(lv, s);
}
});
} catch (InterruptedException | ReactorException | IOException e) {
Copy link

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The short answer is "because the computer did it". A longer answer is that those exceptions don't apply there.

Copy link

Choose a reason for hiding this comment

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

I guess it's fine since they are not runtime ones.

@batmat
Copy link
Member

batmat commented Mar 25, 2019

Let's consider this ready I think.

@batmat batmat added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Mar 25, 2019
@batmat batmat self-requested a review March 25, 2019 17:08
@batmat batmat added the unresolved-merge-conflict There is a merge conflict with the target branch. label Mar 31, 2019
@jsoref
Copy link
Contributor Author

jsoref commented Apr 1, 2019

@batmat: resolved...

@batmat batmat removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 1, 2019
@batmat
Copy link
Member

batmat commented Apr 1, 2019 •
edited

Let's wait for a green PR build and we merge. Because looking at the associated CI, I currently see only red builds BUT all seems either related to the now fixed conflicts, or an infra issue.

I'll check more deeply after it completes:
https://ci.jenkins.io/job/Core/job/jenkins/job/PR-3929/

image

I also just updated the changelog proposal...

@batmat
Copy link
Member

batmat commented Apr 1, 2019

All 💚, merging.

@batmat batmat merged commit 811583e into jenkinsci:master Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvz jvz jvz approved these changes

@varyvol varyvol varyvol approved these changes

@batmat batmat Awaiting requested review from batmat

Assignees
No one assigned
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Milestone
No milestone
4 participants
@jsoref @batmat @jvz @varyvol