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 CreateJobHelper.setup #4078

Closed
wants to merge 1 commit into from

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 jsoref force-pushed the createjobhelper-setup branch from 4673455 to a84e993 Compare June 25, 2019 22:17
@@ -0,0 +1,45 @@
package hudson.cli;
Copy link
Member

Choose a reason for hiding this comment

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

Please use another package for new classes. It is also not clear why a new class would be needed at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no particularly common class between the two existing classes. I'm happy to put it somewhere else. I just need advice.

I used a class because there were two things that were needed and I didn't have a particularly better way to provide both. I'm not used to returning multiple values in Java outside the form of a class. I'm open to suggestions.

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: to be clear, since @daniel-beck thinks you missed it, I was asking you for advice. What package would you suggest?

How would you make a static helper class work?

I'll note that the merge conflicts for this were someone doing identical cleanup. Had this been merged, they would have only needed to perform the cleanup once.

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.

Need in the new class is unclear. Also, automatic IntellijIDEA code generation does not look to be a good solution here. Non-static helper classes is a bad idea IMHO

@fcojfernandez fcojfernandez added 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. labels Jan 3, 2020
@fcojfernandez
Copy link
Member

@oleg-nenashev requested changes on Jul and there has been no activity since then. Besides, there are unresolved merge conflicts, 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

@varyvol
Copy link

varyvol commented Jan 23, 2020

Given there's been no response for nearly one month now, I'm closing this. Please feel free to reopen in case you consider working on this again.

@varyvol varyvol closed this Jan 23, 2020
@jsoref
Copy link
Contributor Author

jsoref commented Mar 17, 2020

@fcojfernandez: the day after you added your commnet, I asked a question of @oleg-nenashev #4078 (comment). Nothing happened until @varyvol closed this.

I was interested then, and I've pushed a merged commit here. -- which people can't see because I can't reopen this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants