-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add ApiVersion parser and comparison #189
Conversation
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.
Looks good to me! Just a couple of clarifying questions. Thanks!
<groupId>com.google.guava</groupId> | ||
<artifactId>guava</artifactId> | ||
<version>32.1.1-jre</version> | ||
</dependency> |
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 know we use Guava widely in other projects so it might not apply here, but wondering if we want to highlight this additional dependency through the new dependency governance process? Or is this already a considered a "safe" dependency that any project is free to import?
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.
CC'ing @suztomo to verify. Guava is an approved dependency to be used in sdk-platform-java, so I've used the same (latest) version here. This project doesn't use java-shared-dependencies... but perhaps we should change that?
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.
This java-docfx-doclet doesn't appear in library users class path. So its dependencies do not matter. I don't see a problem using the shared dependencies.
* │ │ │└─── Optional: Prerelease major version | ||
* │ │ └──── Optional: Stability level. See <a href="https://google.aip.dev/181">AIP 181</a> | ||
* │ └──────── Optional: Minor version | ||
* └────────── Required: Major version |
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.
really helpful description!
} | ||
|
||
@Test | ||
public void testAisGreaterThanB() { |
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 assume this use case would be used when determining which version to recommend?
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.
Partially, yes. The exact logic is now available in PR #190 's ApiVersion#getRecommended method: https://github.com/googleapis/java-docfx-doclet/pull/190/files#diff-e1cb1f1754754c8682e5d8cee8c20a16573edf80ce3c7f7b4f2d08446f51511cR57-R70
Creates an object to parse and compare API versions. Format defined in Javadoc for ApiVersion#parse.