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 CLIReportUnexpectedExceptionHelper #4076

Closed

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
@jsoref
Copy link
Contributor Author

jsoref commented Jun 18, 2019

(This isn't ready, it's split so that it doesn't get lost)

@jsoref jsoref force-pushed the clireportunexpectedexceptionhelper branch from 27e1325 to ee2ec1e Compare June 25, 2019 22:28
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.

No need in a new class, it can be put to CLICommand directly. If there is a justification for such new public helper class, it should be documented properly and put to a new package (jenkins.cli or so)

@daniel-beck daniel-beck added the plugin-api-changes Changes the API of Jenkins available for use in plugins. label Oct 11, 2019
@fcojfernandez
Copy link
Member

@oleg-nenashev requested changes on Jul and there has been no activity since then, so I'm proposing for close. @jsoref please, if you are still interested in this PR, let me know so I can remove the label

@fcojfernandez fcojfernandez 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 Jan 3, 2020
@jsoref
Copy link
Contributor Author

jsoref commented Jan 5, 2020

@fcojfernandez: Sorry, I think when I first worked on this I couldn't figure out how to get the other package to access the higher class. Here we go.

Copy link
Contributor

@zbynek zbynek left a comment

Choose a reason for hiding this comment

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

Whitespace got removed in this PR, IMHO it should be kept (consistency within file and with https://google.github.io/styleguide/javaguide.html#s4.6.2-horizontal-whitespace )

Co-Authored-By: Zbynek Konecny <[email protected]>
@fcojfernandez fcojfernandez removed the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Jan 7, 2020
@rsandell rsandell added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Mar 26, 2020
@daniel-beck daniel-beck removed the plugin-api-changes Changes the API of Jenkins available for use in plugins. label Apr 4, 2020
@daniel-beck
Copy link
Member

Not an API change if @Restricted.

@varyvol varyvol added the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 30, 2020
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.

@jsoref Could you maybe resolve the merge conflict? This seems close to be ready once the conflict is solved.

@jsoref
Copy link
Contributor Author

jsoref commented Apr 30, 2020

The conflicts are with #4641 which did something similar. I'll try to look at it over the weekend.

@daniel-beck
Copy link
Member

The other PR mostly took care of other redundancy IIRC, these should nicely complement each other.

@varyvol
Copy link

varyvol commented Jun 4, 2020

@jsoref I've resolved the conflicts by making the function you introduced call the one introduced under #4641. Could you please confirm you are fine with it?

@varyvol varyvol requested a review from oleg-nenashev June 4, 2020 08:54
@daniel-beck daniel-beck removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Jun 9, 2020
@alecharp
Copy link
Member

alecharp commented Sep 3, 2020

@jsoref Could you please get back to us on this one? It seems it need a bit of work (build failures) but it seems good. Thanks.

@jsoref
Copy link
Contributor Author

jsoref commented Sep 4, 2020

@varyvol: now I remember why I didn't push that. I hit the static issues that the build has now. The functions in question have lost this and thus can't play nicely. I scratched my head and sighed.

@alecharp alecharp added the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 1, 2020
@varyvol varyvol self-requested a review October 1, 2020 13:19
@alecharp
Copy link
Member

alecharp commented Oct 1, 2020

@jsoref do you think you can move this PR forward or should we close it? Thanks.

@alecharp alecharp 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 Oct 15, 2020
@alecharp
Copy link
Member

Adding the proposed-for-close label, unless you have time to work on this @jsoref.

@jsoref
Copy link
Contributor Author

jsoref commented Oct 15, 2020

I really can't figure out a good way to resolve this and I don't have the cycles to poke it.

@jsoref jsoref closed this Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it unresolved-merge-conflict There is a merge conflict with the target branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants