-
Notifications
You must be signed in to change notification settings - Fork 770
Enable Jackson 3 - Phase 2 #2183
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
base: main
Are you sure you want to change the base?
Conversation
Design GitHubJackson interface with methods for reading/writing JSON Implement GitHubJackson2 and GitHubJackson3 Create DefaultGitHubJackson factory with programmatic selection Create GitHubJacksonException wrapper classes Add GitHubBuilder.withJackson() for configuring Jackson implementation Add testing infrastructure for both Jackson versions
| * @see DefaultGitHubJackson#createJackson2() | ||
| * @see DefaultGitHubJackson#createJackson3() | ||
| */ | ||
| public GitHubBuilder withJackson(GitHubJackson jackson) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't have something in the internal package a public method.
I'd suggest just having DefaultGitHubJackson control this and not provide a public method to control this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with an alternative to keep the programmatic access option
| * @throws IllegalStateException | ||
| * if Jackson 3.x is not available on the classpath | ||
| */ | ||
| public GitHubBuilder useJackson3() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment on DefaultGitHubJackson.createDefault():
| public GitHubBuilder useJackson3() { | |
| public GitHubBuilder useJackson2() { |
|
|
||
| private GitHubConnector connector; | ||
|
|
||
| private GitHubJackson jackson; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private GitHubJackson jackson; | |
| private GitHubJackson jackson = DefaultGitHubJackson.createDefault(); |
| rateLimitChecker, | ||
| authorizationProvider); | ||
| authorizationProvider, | ||
| jackson != null ? jackson : DefaultGitHubJackson.createDefault()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| jackson != null ? jackson : DefaultGitHubJackson.createDefault()); | |
| jackson); |
| * @return a GitHubJackson2 instance | ||
| */ | ||
| public static GitHubJackson createDefault() { | ||
| return new GitHubJackson2(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should default to Jackson3 if it is available, but I'm open to discussion.
| return new GitHubJackson2(); | |
| if (isJackson3Available()) { | |
| return new GitHubJackson3(); | |
| } else { | |
| return new GitHubJackson2(); | |
| } |
Description
See #2173 for the initial PR and associated discussion.
This PR is the follow-up of #2182.
Before submitting a PR:
@linkJavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .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.main. You will create your PR from that branch.When creating a PR: