-
Notifications
You must be signed in to change notification settings - Fork 4.5k
make use of generic type info in *Utils classes #36581
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
make use of generic type info in *Utils classes #36581
Conversation
Summary of ChangesHello @tilgalas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request is the fourth in a series aimed at enabling full support for generic classes within Beam's schema providers. It focuses on refining how type information is accessed and utilized across various utility classes responsible for schema-related operations. By transitioning from potentially incomplete Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #36581 +/- ##
=========================================
Coverage 56.93% 56.93%
- Complexity 3393 3395 +2
=========================================
Files 1222 1222
Lines 186813 186817 +4
Branches 3543 3545 +2
=========================================
+ Hits 106362 106373 +11
+ Misses 77079 77073 -6
+ Partials 3372 3371 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
Run Java PreCommit |
|
Run Java_Amazon-Web-Services2_IO_Direct PreCommit |
|
Run Java_IOs_Direct PreCommit |
|
Assigning reviewers: R: @chamikaramj for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Reminder, please take a look at this pr: @chamikaramj |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @kennknowles for label java. Available commands:
|
kennknowles
left a comment
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 mostly seems fine, I'm just still catching up on the design doc and everything and the new FieldValueTypeInformation vs TypeDescriptor. So I still have more like high level curiosity.
| typeInformation | ||
| .getRawType() | ||
| .isAssignableFrom( | ||
| Preconditions.checkNotNull(typeInformation.getMethod()).getReturnType()) |
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.
Please avoid Preconditions.checkNotNull - we've made better versions. Please use checkArgumentNotNull if the null error should be blamed on the caller, or checkStateNotNull if the null error should be blamed on this class.
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.
got it, will fix
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.
pushed an update - I had to change some additional lines containing the old-style checkNotNull invocations which made the PR somewhat larger- hopefully it's still relatively easy to scan those changes while doing the review
sure, thanks so much! |
9eebd65 to
050369f
Compare
|
Run Java PreCommit |
1 similar comment
|
Run Java PreCommit |
|
Reminder, please take a look at this pr: @kennknowles |
This is a 4th PR in the series of PRs (based on the now closed #31648) whose ultimate goal is to add support for generic classes to schema providers.
This PR replaces the uses of incomplete type information carried in
TypeDescriptor.ofinvocations in the various *Utils classes with the full type signature stored in theFieldValueTypeInformationobjects