Migrate legacy date-time api to new date-time api

[solonovamax]

Description

Migrates the legacy date-time api to newer date-time api introduced in JSR 310.

Fixes #2073

Did not introduce any new tests for this, as no new functionality was introduced, only updated the previous tests to use Instant instead of Date.

Several methods with date in the name were renamed to instead have instant in the name. All of these methods are package-private or only accessible to the tests.

Before submitting a PR:

• Changes must not break binary backwards compatibility. If you are unclear on how to make the change you think is needed while maintaining backward compatibility, CONTRIBUTING.md for details. These changes break binary backwards compatibility, however this is meant for new 2.x release.
• Add JavaDocs and other comments explaining the behavior.
• When adding or updating methods that fetch entities, add @link JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .
• Add tests that cover any added or changed code. This generally requires capturing snapshot test data. See CONTRIBUTING.md for details.
• Run mvn -D enable-ci clean install site "-Dsurefire.argLine=--add-opens java.base/java.net=ALL-UNNAMED" locally. If this command doesn't succeed, your change will not pass CI.
• Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

• Fill in the "Description" above with clear summary of the changes. This includes:
  • If this PR fixes one or more issues, include "Fixes #" lines for each issue.
  • Provide links to relevant documentation on https://docs.github.com/en/rest where possible. If not including links, explain why not.
• All lines of new code should be covered by tests as reported by code coverage. Any lines that are not covered must have PR comments explaining why they cannot be covered. For example, "Reaching this particular exception is hard and is not a particular common scenario."
• Enable "Allow edits from maintainers".

[solonovamax]

Also a few things I want to mention:

• when trying to create the PR through IntelliJ, it did not let me due to a configuration setting on your end. I was trying to create a PR from my fork to this repo, so it's not an issue with not having access rights to the repository, that is a separate message. The error message was

  Failed to create a pull request
  Although you appear to have the correct authorization credentials, the hub4j organization has enabled OAuth App access restrictions, meaning that the data access to third-parties is limited. For more information on these restrictions, including how to enable this app, visit https://docs.github.com/articles/resstricting-access-to-your-organization-s-data/

• I noticed a few inconsistencies in the source code:
  • some methods have underscores in their names (this is something that would be best to fix right now before the 2.x release)
    • GHRelease.getPublished_at()
    • GHDeployKey.getAdded_by()
    • GHDeployKey.isRead_only()
    • GHDiscussion.private_()
    • GHGistBuilder.public_()
    • GHPullRequestCommitDetail.getComment_count()
    • GHPullRequestCommitDetail.getHtml_url()
    • GHRepositoryBuilder.private_()
  • some classes have a license header at the top, others do not
  • some classes use glob imports, others do not

[bitwiseman]

You observations are all valid.

• The intelliJ problem I'm open to changing settings if it is needed. File a separate issue.
• The license problem has an open issue. Not sure how to enforce it yet, but we should.
• The underscore problem - please file an issue to add an ArchUnit test that enforces no underscores (with the ability to add exceptions for cases we choose to allow)
• I also need to set naming guidance for future additions to the library.
• We use spotless plugin to enforce formatting and depend on it to keep imports clean. There's probably a setting we could add to enforce usage either for or against globs. Again please file an issue.

[bitwiseman]

As for your code changes, thanks for the fast turnaround. The build failed because you introduced binary breaking changes. That is not a surprise. Other than that, it looks good.

I know we're moving to 2.0, but I'd still like to minimize breaking charges. (Lengthy discussion here: #1938) So, I'd like to use depreciation annotations or bridge methods to address this.

If we use depreciation , I'd say we name the new Instant methods without the get on front. Have the Date methods call the Instant methods, and then we can leave the tests unchanged and still get full coverage.

We previously handled these kind of API breaking changes using bridge methods. One advantage of using bridge methods over depreciation annotations is it forces people to switch to using the new API the next time they compile their library (not at some future point). The disadvantage is they make mocking tests fail in odd ways. Still, we just removed them, I was kind of hoping to avoid bringing them back.

Sigh. I guess we can reenable producing an -unbridged artifact for the 2.x release line. It's minor added cost.

So, I'm leaning towards bridge methods. What are your thoughts?

(FYI: I don't expect you to make these changes either way, but I'd appreciate your help if you have time.)

[solonovamax]

As for your code changes, thanks for the fast turnaround. The build failed because you introduced binary breaking changes. That is not a surprise. Other than that, it looks good.

yeah, I gathered that much.
I was going to comment about it, but forgot to

all the tests I was able to run locally all passed (a few were skipped, most likely bc they're windows or macos specific tests & I'm on linux)

We previously handled these kind of API breaking changes using bridge methods. One advantage of using bridge methods over depreciation annotations is it forces people to switch to using the new API the next time they compile their library (not at some future point). The disadvantage is they make mocking tests fail in odd ways. Still, we just removed them, I was kind of hoping to avoid bringing them back.

wdym by "bridge methods"?

The intelliJ problem I'm open to changing settings if it is needed. File a separate issue.

sure, I can do that.

The license problem has an open issue. Not sure how to enforce it yet, but we should.

looking at spotless, it seems that it has a check for the license header. so, if you wanted to enforce it you could use that.
see: https://github.com/diffplug/spotless/tree/main/plugin-maven#license-header

The underscore problem - please file an issue to add an ArchUnit test that enforces no underscores (with the ability to add exceptions for cases we choose to allow)

sure, I can look into that in a bit.

We use spotless plugin to enforce formatting and depend on it to keep imports clean. There's probably a setting we could add to enforce usage either for or against globs. Again please file an issue.

sure, will do.
it seems that there is currently an open issue on removing wildcard imports: diffplug/spotless#649
it does propose a solution of

<replaceRegex>
    <name>Remove wildcard imports</name>
    <searchRegex>import\s+(?:static\s+)?[^\*\s]+\*;(\r\n|\r|\n)</searchRegex>
    <replacement>$1</replacement>
</replaceRegex>

though, simply running this will cause the compile to fail because it doesn't replace the glob import with imports for the used classes/methods.

[solonovamax]

also something else I just noticed, is that it seems in some places all local variables (which are not later modified) are declared as final, whereas in other places they are not.

[bitwiseman]

also something else I just noticed, is that it seems in some places all local variables (which are not later modified) are declared as final, whereas in other places they are not.

Yeah, there isn't really one right answer and it can be confusing for new or occasional contributors.

See this example for pros/cons of different options:
https://github.com/hub4j/github-api/blob/main/src%2Fmain%2Fjava%2Forg%2Fkohsuke%2Fgithub%2Fexample%2Fdataobject%2FReadOnlyObjects.java

[bitwiseman]

wdym by "bridge methods"?

Bridge methods are binary method overloads for methods with the same arguments (or no arguments) and different return types.

Look at https://github.com/jenkinsci/bridge-method-injector

You can see examples of how we used them by looking at the main-1.x branch.

[solonovamax]

also something else I just noticed, is that it seems in some places all local variables (which are not later modified) are declared as final, whereas in other places they are not.

Yeah, there isn't really one right answer and it can be confusing for new or occasional contributors.

See this example for pros/cons of different options: https://github.com/hub4j/github-api/blob/main/src%2Fmain%2Fjava%2Forg%2Fkohsuke%2Fgithub%2Fexample%2Fdataobject%2FReadOnlyObjects.java

I'm not talking about the fields/classes, I'm talking about, for example (GHSearchBuilder):

GHQueryBuilder<T> q(@Nonnull final String qualifier, @CheckForNull final String value) {
    if (StringUtils.isEmpty(qualifier)) {
        throw new IllegalArgumentException("qualifier cannot be null or empty");
    }
    if (StringUtils.isEmpty(value)) {
        final String removeQualifier = qualifier + ":";
        terms.removeIf(term -> term.startsWith(removeQualifier));
    } else {
        terms.add(qualifier + ":" + value);
    }
    return this;
}

[solonovamax]

wdym by "bridge methods"?

Bridge methods are binary method overloads for methods with the same arguments (or no arguments) and different return types.

Look at https://github.com/jenkinsci/bridge-method-injector

You can see examples of how we used them by looking at the main-1.x branch.

honestly, I feel like the breaking changes are rather small. if anyone is still heavily relying on the old date api, then they can either just wrap it in Date.from(...) or they just don't upgrade to the newer api.

I guess the one complication of this would be jenkins plugins, as I believe that this api is used in several different locations, and many jenkins plugins are not exactly being maintained (eg. I have jenkinsci/discord-notifier-plugin#138 and jenkinsci/github-branch-source-plugin#790 which have just been sitting there for nearly a year lol, one of which has even forced me to use my own build of the plugin otherwise rendering jenkins entirely unusable)

[bitwiseman]

Oh, local variables. Yeah, I have no idea or opinion there. There are reasons to declare final, but method doesn't have them. File an issue and we can discuss there.

And yes the binary compatibility requirement comes from Jenkins. It's a long discussion linked above but it comes down to we need to give them more lead time to handle changes.

Maybe the right thing to do here is to have the primary artifact be unbridged (with no added descriptor) and have a secondary "-bridged" artifact?

We definitely need to make all bridge methods log warnings to get people to move to the new methods.

[solonovamax]

Maybe the right thing to do here is to have the primary artifact be unbridged (with no added descriptor) and have a secondary "-bridged" artifact?

yeah, this might be the best option

I feel like the majority of consumers of the library will have little to no use for the bridged methods and will just be able to migrate without significant issue.
then again, for those who don't have any use for the bridged methods, having them there causes no harm, correct?

[bitwiseman]

Actually, we created the unbridged because there were issues with consumers of this library mocking classes that had bridge methods. 😭

[codecov]

Codecov Report

Attention: Patch coverage is 97.97980% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.42%. Comparing base (b8d48f3) to head (b2d1420).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../java/org/kohsuke/github/GHNotificationStream.java	75.00%	1 Missing ⚠️
src/main/java/org/kohsuke/github/GHRateLimit.java	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2074      +/-   ##
============================================
+ Coverage     83.84%   84.42%   +0.57%     
- Complexity     2414     2455      +41     
============================================
  Files           236      237       +1     
  Lines          7330     7344      +14     
  Branches        389      388       -1     
============================================
+ Hits           6146     6200      +54     
+ Misses          952      915      -37     
+ Partials        232      229       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[solonovamax]

Noticed you forgot to mark a few things as @Deprecated

unsure if this is intentional or not, so I'm not going to push a commit that changes it, but if it wasn't then that's why I'm commenting on it

[bitwiseman]

@solonovamax
It was not intentional just WIP.
Updated.

[bitwiseman]

@solonovamax
I'm done making changes.

[bitwiseman]

Yes, I know it is odd to be approving a change I worked on. Oh well.