-
Notifications
You must be signed in to change notification settings - Fork 352
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 ODataJsonLightSerializer #2030
Implement asynchronous support in ODataJsonLightSerializer #2030
Conversation
86d7704
to
225286f
Compare
b16b4d7
to
2be0ac4
Compare
2be0ac4
to
ffb0111
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.
LGTM
src/Microsoft.OData.Core/JsonLight/ODataJsonLightValueSerializer.cs
Outdated
Show resolved
Hide resolved
/// </summary> | ||
/// <param name="property">The property to write.</param> | ||
/// <returns>A task that represents the asynchronous write operation.</returns> | ||
internal Task WriteTopLevelPropertyAsync(ODataProperty property) |
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.
Task [](start = 17, length = 4)
why no async key word?
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.
@xuzhg Eliding is recommended when a method is a pass-through. See this article on the subject
https://blog.stephencleary.com/2016/12/eliding-async-await.html#recommended-guidelines:~:text=Recommended%20Guidelines
This method qualifies as a pass-through since it merely calls another method to do the job.
Eliding will save us the overhead of creating and later garbage-collecting the state machine.
For that reason, I'm eliding the async
and await
whenever the method is just a pass-through
@@ -12,6 +12,7 @@ | |||
using Microsoft.OData.JsonLight; | |||
using Microsoft.OData.Edm; | |||
using Xunit; | |||
using System.Threading.Tasks; |
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.
System.Threading.Tasks; [](start = 6, length = 23)
sort #Resolved
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 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 a partial fulfilment of issue #2019.
Description
Implement asynchronous support in
ODataJsonLightSerializer
,ODataJsonLightValueSerializer
,ODataJsonLightPropertySerializer
andJsonLightInstanceAnnotationWriter
Disclaimer:
This PR ended up affecting 5 classes due to dependencies that were not easy to go around without ending up with an incomplete/non-functional API. For the following reasons:
ODataJsonLightSerializer
class has a hard-coded dependency onJsonLightInstanceAnnotationWriter
and invokes particular methods to write instance annotations.JsonLightInstanceAnnotationWriter
class has a hard-coded dependency onODataJsonLightSerializer
andODataJsonLightValueSerializer
classes and invokes methods from both classes in the course of writing instance annotations.ODataJsonLightValueSerializer
class has a hard-coded dependency onODataJsonLightPropertySerializer
classODataJsonLightValueSerializer
andODataJsonLightPropertySerializer
subclass ODataJsonLightSerializerChecklist (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.