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 #4034

Closed
wants to merge 9 commits into from
Closed

Java cleanup #4034

wants to merge 9 commits into from

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented May 17, 2019

This is for discussion.

Notes:

  • Most commits were created by IntelliJ
  • Individual commits are mostly self contained
  • In general, most commits should not change program behavior at all

This is generally a subset of https://github.com/jsoref/jenkins/commits/java-cleanup-full

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

jsoref added 8 commits May 14, 2019 15:14
@jsoref
IntelliJ/Java: Duplicate declaration
dfbe6b4 
AntClassLoader.getUrl
@jsoref
IntelliJ/Java: Duplicate declaration
0e2b395 
ApiTokenProperty.checkPermission
@jsoref
IntelliJ/Java: Duplicate code
906ca05 
Refactored as ChunkedInputStream.advanceChunk
@jsoref
IntelliJ/Java: Duplicate code
311bdb5 
Reused Computer.resolveForCLI
@jsoref
IntelliJ/Java: Duplicate code
41292ac 
Refactored as SettingsPath.getSettings
@jsoref
IntelliJ/Java: Duplicate code
729e347 
Refactored as Run.getBuildsOverThreshold
@jsoref
IntelliJ/Java: Duplicate code
07e53b1 
Refactored as View.makeSearchIndex
@jsoref
IntelliJ/Java: Duplicate code
e6ec9ba 
Refactored as PluginManager.logPluginWarnings
@jsoref jsoref force-pushed the java-cleanup branch 2 times, most recently from 836bdf1 to 322e9f0 Compare June 17, 2019 02:50
Wadeck
Wadeck reviewed Jun 17, 2019
View reviewed changes
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.

After partial review, I am not sure to see the interest in adding complexity with new helper classes for case that are duplicate along different files.

Also some concerns about the restriction and the return code for CLI.

No blocker for me, but neither an approval ;)

@jsoref jsoref force-pushed the java-cleanup branch 3 times, most recently from 89dc9b6 to 948acc8 Compare June 17, 2019 21:05
Wadeck
Wadeck requested changes Jun 18, 2019
View reviewed changes
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.

Most commits were created by IntelliJ
Could you elaborate on which parts are made by IntelliJ and which are "human created"?

I tried to add some suggestions, but they are not useful because you need to rename the use of the classes as well.

⚠️ For the abstract static methods, please change the behavior, it's broken.

Due to the huge amount of changes here, I cannot support/help more without spending too much time.

oleg-nenashev
oleg-nenashev requested changes Jun 18, 2019
View reviewed changes
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.

Could you please breakdown the PR to multiple ones?
I see several changes there (just refactoring, using class index and so on), and I do not think it fits a single PR

@jsoref
Copy link
Contributor Author

jsoref commented Jun 18, 2019

@oleg-nenashev: This should now just contain the boring portions.

This was referenced Jun 18, 2019
Merged
Closed
Closed
Closed
Merged
Merged
@oleg-nenashev oleg-nenashev self-requested a review June 28, 2019 20:01
oleg-nenashev
oleg-nenashev reviewed Jun 28, 2019
View reviewed changes
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.

Looks good to me overall. New API methods (included protected) should be properly documented before merging

@jsoref
Copy link
Contributor Author

jsoref commented Jun 28, 2019

Thanks @oleg-nenashev; changes applied.

oleg-nenashev
oleg-nenashev requested changes Jul 9, 2019
View reviewed changes
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.

New API methods (included protected) should be properly documented before merging

@jsoref
Copy link
Contributor Author

jsoref commented Jul 9, 2019

@oleg-nenashev: updated -- I also dropped a param, I'm hoping that works...

@Wadeck Wadeck requested review from Wadeck and removed request for Wadeck July 10, 2019 07:24
Wadeck
Wadeck reviewed Jul 10, 2019
View reviewed changes
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.

Due to the force-push, I cannot see my initial request being corrected or not.

oleg-nenashev
oleg-nenashev reviewed Jul 10, 2019
View reviewed changes
core/src/main/java/hudson/model/Run.java
* if not enough builds satisfying the threshold have been found. Never null.
* @since TODO
*/
protected @Nonnull List<RunT> getBuildsOverThreshold(RunT r, int numberOfBuilds, @Nonnull Result threshold) {
Copy link
Member

Choose a reason for hiding this comment

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

That could be even made a public API now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want it to be, sure. I'm trying not to change public api surfaces in these PRs.

Copy link
Member

Choose a reason for hiding this comment

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

protected is also a public API, so no big difference 🤷‍♂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, well, if people are ok w/ switching to public, I don't mind, it seemed like a reasonable improvement, I'm just not terribly familiar w/ how jenkins does api design.

@jsoref @oleg-nenashev
Warn to check for null
4c251cc 
Co-Authored-By: Oleg Nenashev <[email protected]>
@rsandell rsandell added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Sep 5, 2019
@fcojfernandez
Copy link
Member

@jsoref this PR has been proposed for close. First of all, sorry for not pinging you previously because you didn't receive any notification when the label was added.
Apart from the label, there are change requests from @Wadeck and @oleg-nenashev . Do you plan to continue working on this PR?

@Wadeck Wadeck requested review from Wadeck and removed request for Wadeck October 3, 2019 10:19
@Wadeck
Copy link
Contributor

Wadeck commented Oct 3, 2019

Like previous comment, I cannot review the changes, so I dunno if my change requests were applied or not. No time to look again at it. I cannot remove myself from the review list without being again "requesting a change" -.-

@jsoref jsoref closed this Oct 3, 2019
@jsoref
Copy link
Contributor Author

jsoref commented Oct 3, 2019

Split as:
#4254
#4255
#4256
#4257
#4258
#4259
#4260
#4261

@jsoref jsoref deleted the java-cleanup branch October 3, 2019 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev oleg-nenashev requested changes

@Wadeck Wadeck Awaiting requested review from Wadeck

Assignees
No one assigned
Labels
proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jsoref @fcojfernandez @Wadeck @oleg-nenashev @rsandell