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

[FIXED JENKINS-36593] - Make ItemCategory#MIN_TOSHOW restricted #2449

Conversation

oleg-nenashev
Copy link
Member

@ghost
Copy link

ghost commented Jul 12, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@recena
Copy link
Contributor

recena commented Jul 12, 2016

🐝 although this field was introduced in middle of a long process and my recommendation today is to remove it. Really, the service consumer (front-end for example) is who should decide the rendering conditions.

@recena
Copy link
Contributor

recena commented Jul 12, 2016

@oleg-nenashev How a user is able to change this static field?

@recena
Copy link
Contributor

recena commented Jul 12, 2016

@oleg-nenashev I'm considering to remove my bee 😄

@oleg-nenashev
Copy link
Member Author

@recena Non-final static fields can be easily changed. E.g. by invoking ItemCategory.MIN_TOSHOW = 5

@recena
Copy link
Contributor

recena commented Jul 12, 2016

@oleg-nenashev I don't see the problem in this case. This value is only a default value, as a reference to the developer of ItemCategory.

@oleg-nenashev
Copy link
Member Author

Just to summarize the problem, public static fields are evil from the API maintenance perspective. Once you introduce and release the field, there is no real way back. For example, you cannot remove this field without a formal binary compatibility breakage in the Jenkins core.

@Restricted at least allows avoiding explicit usages in other plugins.

@recena
Copy link
Contributor

recena commented Jul 12, 2016

@oleg-nenashev I understand what you try to achieve but I hope some day, Jenkins provides a policy to deprecate and remove methods, fields, etc.

@oleg-nenashev
Copy link
Member Author

This API deprecation engine originally was in 2.0 scope. I'm not sure if
anybody plans to work on it in short-term. Likely no. But I agree that we
cannot move forward easily without API deprecation
On Jul 12, 2016 8:48 AM, "Manuel Recena" notifications@github.com wrote:

@oleg-nenashev https://github.com/oleg-nenashev I understand what you
try to achieve but I hope some day, Jenkins provides a policy to deprecate
and remove methods, fields, etc.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2449 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AC3IoJwsZUhoYZqXp3ctozFX_BNBC3fhks5qUzjKgaJpZM4JKDoR
.

@oleg-nenashev
Copy link
Member Author

@recena Are you fine with the explanation?

@recena
Copy link
Contributor

recena commented Jul 15, 2016

@oleg-nenashev Sure.

@oleg-nenashev
Copy link
Member Author

@reviewbybees done
Merging it since the author of the original commit is fine with the change

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jul 15, 2016
@oleg-nenashev oleg-nenashev merged commit 4577cf6 into jenkinsci:master Jul 15, 2016
oleg-nenashev added a commit that referenced this pull request Jul 17, 2016
olivergondza pushed a commit that referenced this pull request Jul 21, 2016
* [JENKINS-36593] - ItemCategory#MIN_TOSHOW should be restricted

* [JENKINS-36593] - Add Javadoc

(cherry picked from commit 4577cf6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

2 participants