Demonstrate fix of collection typing#119
Conversation
This comment has been minimized.
This comment has been minimized.
- fixes generic arguments passing - removes unecessary parameters
- fixes a bug where requests would not be built because of wrong constructor search
- updates link to documentation
- removes unecessary unchecked suppression - further defensive programming
|
Here is a bit of context for the review. I couldn't go "all the way" removing things like collection pages, collection response etc... as the java generic type erasure "looses some information" if you don't have a type inheriting from the generic type. The corresponding typewriter PR is here microsoftgraph/MSGraph-SDK-Code-Generator#313 The changes of this generation have been run against raptor here. It's complaining about the todo stuff which is normal as I haven't updated the feature/v3 branch from latest metadata in a while. Besides that raptor allowed me to catch one generation + generic issue as you can see in the latest commits. |
MIchaelMainer
left a comment
MIchaelMainer
left a comment
There was a problem hiding this comment.
We had a review meeting online to discuss the changes. I'll follow up after I've done some testing of these changes
| import java.util.Map; | ||
|
|
||
| /** | ||
| * The base method request builder class used for POST actions |
There was a problem hiding this comment.
The base method request builder class used for OData Action POST requests that return a collection.
The naming led me to believe that this is bound to a collection, and not referring to the return type.
There was a problem hiding this comment.
is there an action to take on that?
There was a problem hiding this comment.
Yeah, the request is to change the comment to proposed comment.
There was a problem hiding this comment.
| * The base method request builder class used for POST actions | |
| * The base method request builder class used for OData Action POST requests that return a collection. |
This comment has been minimized.
This comment has been minimized.
|
@zengin @MIchaelMainer FYI we do have the type summary working now, it contains a lot of changes but as far as I can tell this PR is not removing anything. Please have a look whenever you have some time. |
4 participants
related #116
fixes #25
fixes #31
fixes #120
This is what I've done:
BaseCollectionRequestto have only one generic parameter TresponseClassthat extendsICollectionResponse<T>ICollectionResponsesendnow returns anICollectionResponse<T>I've just addressed the T1 class in the constructor. Handling the T2 involves a little more but is similar.
Perhaps this will help you understand the changes proposed.