Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#907: Enable read the untyped collection #905
#907: Enable read the untyped collection #905
Changes from all commits
271380f
22a72c4
9c91d77
c07a245
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can
itemStack
containnull
entries? Or does this assertion (and others like it in this class) make the assumption that the cast will returnnull
if the conversion fails? If the item at the top of the stack is not the expected type, anInvalidCastException
will be thrown. If we expect null, we should consider usingitemStack.Peek() as ODataNestedResourceInfoWrapper
.But I don't mind throwing the exception if that was by design.
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 this doesn't apply to the new code change, but I've seen it in other places in this class and thought it's worth pointing out.
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.
ODL reading logic guarantees so far.
In my plan, I will retrieve the reading as a service and customer can customize the reading process. It's a future feature.
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.
The guarantee makes sense. My comment was more on the fact that
Contact.Assert
verifies that theparentNestedResourceInfo
is notnull
. However, if the cast would have failed,parentNestedResourceInfo
would not have been set to null, an exception would have been thrown, and therefore theContact.Assert
would not have been reached.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.
itemsStack.Peek() could return 'null', I think.
ODataNestedResourceInfoWrapper parentNestedResourceInfo = (ODataNestedResourceInfoWrapper)null;
is valid. We can sync it more, but it's no scope of this PR. :)
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.
Is there supposed to be code somewhere that reads the
Items
property to actually populate the CLR type?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.
What do you mean? @corranrogue9
To add a Kind property in the base type and implement to return different kind in different wrapper?
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 also have a similar question:
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.
Since 'Resources' is open/public, we can't stop customers adding new resources into that.
Same reason, we can't stop customers to modify the 'Items'.
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.
Items
is being introduced in this PR, so it doesn't exist in existing code. So my question was whether we need to expose it to customers and why.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.
Customer may override the 'CreateResourceSet' method in ODataResourceSetDeserializer to access the 'Items'.
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 see.