-
Notifications
You must be signed in to change notification settings - Fork 349
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
Implement asynchronous support in ODataJsonLightDeltaWriter #2082
Implement asynchronous support in ODataJsonLightDeltaWriter #2082
Conversation
cf884b5
to
dc25792
Compare
dc25792
to
7ee5683
Compare
@@ -1597,7 +2636,6 @@ private void WriteDeltaLinkTarget(ODataDeltaLinkBase link) | |||
/// Validates that the ODataResourceSet.InstanceAnnotations collection is empty for the given expanded resource set. | |||
/// </summary> | |||
/// <param name="resourceSet">The expanded resource set in question.</param> | |||
[SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Justification = "An instance field is used in a debug assert.")] |
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.
Why are we deleting the SuppressMessage?
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.
Because the Debug.Assert
statements refer to this
so I reasoned that the "MarkMembersAsStatic" warning would no longer valid.
/// <summary> | ||
/// This method is called when an async stream is requested. It is a no-op. | ||
/// </summary> | ||
/// <returns>A task for method called when a stream is requested.</returns> | ||
Task IODataStreamListener.StreamRequestedAsync() | ||
{ | ||
return TaskUtils.GetTaskForSynchronousOperation(() => ((IODataStreamListener)this).StreamRequested()); | ||
return TaskUtils.CompletedTask; |
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 remove the IODataStreamListener.StreamRequested()
if it's not needed anymore
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.
It'd not be safe to remove the method since this class implements IODataOutputInStreamErrorListener
hence the requirement to implement all the methods including StreamRequested
and StreamRequestedAsync
. Its actually an extensibility hook, just like StreamDisposed
and StreamDisposedAsync
@@ -2374,11 +2629,6 @@ private void EnterScope(WriterState newState, ODataItem item) | |||
nestedResourceInfo.IsCollection = navigationProperty.Type.IsEntityCollectionType(); | |||
} | |||
|
|||
if (!nestedResourceInfo.IsCollection.HasValue) | |||
{ |
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.
Why was this code removed?
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.
@KenitoInc Because the block was repeated
17618c2
to
da73cc9
Compare
da73cc9
to
2c8c632
Compare
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; might need a test for OnInStreamErrorAsync
2c8c632
to
00f0c11
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Issues
This pull request is in partial fulfilment of issue #2019.
Description
Implement asynchronous support in
ODataJsonLightDeltaWriter
Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.