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

Deprecate TimeUnit2 #2892

Merged
merged 5 commits into from Sep 18, 2017

Conversation

6 participants
@batmat
Member

batmat commented May 19, 2017

java.util.concurrent.TimeUnit is preferrable now.

Quoting Stephen:

Java 5 did not have all the units required. So TU2 was one that had better conversion. Can be deprecated
once we upgrade to Java 6 ;-)

Description

  • I didn't file a JENKINS issue since this is a pretty internal/dev oriented change. But if this is judged necessary, please tell me so and I'll do it.

Details: see title, very small change. And 0 risk from a runtime perspective.

Changelog entries

Proposed changelog entries:

N/A I don't even think this should have an entry in the changelog, should it?

Submitter checklist

  • [ ] JIRA issue is well described
  • [ ] Link to JIRA ticket in description, if appropriate
  • Appropriate autotests or explanation to why this change has no tests
  • [ ] For new API and extension points: Link to the reference implementation in open-source (or example in Javadoc)

Desired reviewers

@reviewbybees

Deprecate TimeUnit2
java.util.concurrent.TimeUnit is preferrable now.

Quoting Stephen:
"Java 5 did not have all the units required. So TU2 was
one that had better conversion. Can be deprecated once
we upgrade to Java 6 ;-)"
@oleg-nenashev

🐝

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 19, 2017

Member

Maybe @Restricted(DoNotUse.class) will be a good addition

Member

oleg-nenashev commented May 19, 2017

Maybe @Restricted(DoNotUse.class) will be a good addition

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees May 19, 2017

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.

reviewbybees commented May 19, 2017

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.

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 19, 2017

Member

@stephenc @oleg-nenashev @Restricted(DoNotUse.class) also added as requested/proposed.

Member

batmat commented May 19, 2017

@stephenc @oleg-nenashev @Restricted(DoNotUse.class) also added as requested/proposed.

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 19, 2017

Member

Heh, I forgot something pretty important: that adding the restriction implied to replace the code using it to TimeUnit too :)

Member

batmat commented May 19, 2017

Heh, I forgot something pretty important: that adding the restriction implied to replace the code using it to TimeUnit too :)

@jglick

There are dozens of usages of TimeUnit2 in this repository. You should clean them up. (I know they are harmless, but as a matter of policy we should not be deprecating things with a claim that a full replacement is available, without proof that this is actually so in some representative code base.)

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 19, 2017

Member

Agreed @jglick I actually have that change locally, but it fails for now because accmod either is limited or, more probably IIUC, has a bug wrt to enums.

[INFO] --- access-modifier-checker:1.11:enforce (default) @ jenkins-core ---
[ERROR] hudson/util/TimeUnit2$1:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$2:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$7:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$4:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$3:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$6:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$5:0 hudson/util/TimeUnit2 must not be used
Member

batmat commented May 19, 2017

Agreed @jglick I actually have that change locally, but it fails for now because accmod either is limited or, more probably IIUC, has a bug wrt to enums.

[INFO] --- access-modifier-checker:1.11:enforce (default) @ jenkins-core ---
[ERROR] hudson/util/TimeUnit2$1:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$2:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$7:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$4:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$3:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$6:0 hudson/util/TimeUnit2 must not be used
[ERROR] hudson/util/TimeUnit2$5:0 hudson/util/TimeUnit2 must not be used
@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick May 19, 2017

Member

Arguably a bug in access-modifier-checker. You could fix that and deploy a snapshot for use in this PR, or just relax to NoExternalUse for now.

Member

jglick commented May 19, 2017

Arguably a bug in access-modifier-checker. You could fix that and deploy a snapshot for use in this PR, or just relax to NoExternalUse for now.

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 20, 2017

Member

@jglick fixed, build is passing on my machine.
It's for now using a SNAPSHOT of kohsuke/access-modifier#6 while it's not merged/released.

Member

batmat commented May 20, 2017

@jglick fixed, build is passing on my machine.
It's for now using a SNAPSHOT of kohsuke/access-modifier#6 while it's not merged/released.

@daniel-beck daniel-beck added the on-hold label May 20, 2017

Show outdated Hide outdated core/src/main/java/hudson/util/TimeUnit2.java
Show outdated Hide outdated pom.xml
@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Sep 15, 2017

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.

reviewbybees commented Sep 15, 2017

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.

Downgrade restriction from DoNotUse to NoExternalUse
There's a failure with accmod using DoNotUse. So taking the
easy path here to move forward. This will already have the intended
effect: clarify that TimeUnit2 is not to be used anymore.
@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat Sep 17, 2017

Member

Finally followed the recommendation of @jglick above:

[...] just relax to NoExternalUse for now.

In the end, the goal of actually deprecating TimeUnit2 and that people/code start adapting and replace their usage would be moving forward, which was the gist of the PR...

Sorry for the long-lived PRs :-(

Member

batmat commented Sep 17, 2017

Finally followed the recommendation of @jglick above:

[...] just relax to NoExternalUse for now.

In the end, the goal of actually deprecating TimeUnit2 and that people/code start adapting and replace their usage would be moving forward, which was the gist of the PR...

Sorry for the long-lived PRs :-(

@batmat batmat requested a review from jglick Sep 17, 2017

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat
Member

batmat commented Sep 18, 2017

@batmat batmat merged commit 112ceeb into jenkinsci:master Sep 18, 2017

1 check was pending

continuous-integration/jenkins/pr-head This commit is being built
Details

@batmat batmat deleted the batmat:deprecate-timeunit2 branch Sep 18, 2017

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat Sep 18, 2017

Member

Thanks for the reviews!

Member

batmat commented Sep 18, 2017

Thanks for the reviews!

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Sep 24, 2017

Member

Since TimeUnit2 is used in plenty of plugins, and newly restricted core API, a changelog entry is definitely needed.

Member

daniel-beck commented Sep 24, 2017

Since TimeUnit2 is used in plenty of plugins, and newly restricted core API, a changelog entry is definitely needed.

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat Sep 24, 2017

Member

@daniel-beck

  • hudson.util.TimeUnit2 deprecated, use java.util.concurrent.TimeUnit instead.

?

Or longer version:

  • hudson.util.TimeUnit2 deprecated, use java.util.concurrent.TimeUnit instead. TimeUnit2 was introduced to compensate some TimeUnit shortcomings in JDK 5. Those limitations are fixed since JDK 6.
Member

batmat commented Sep 24, 2017

@daniel-beck

  • hudson.util.TimeUnit2 deprecated, use java.util.concurrent.TimeUnit instead.

?

Or longer version:

  • hudson.util.TimeUnit2 deprecated, use java.util.concurrent.TimeUnit instead. TimeUnit2 was introduced to compensate some TimeUnit shortcomings in JDK 5. Those limitations are fixed since JDK 6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment