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.

Sign up for GitHub

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

Jump to bottom

Implement asynchronous support for ODataJsonLightParameterWriter #2096

Merged

gathogojr merged 1 commit into OData:master from gathogojr:feature/odatajsonlightparameterwriter-async

Jun 11, 2021

Contributor

gathogojr commented May 21, 2021 •

edited

Loading

Issues

This pull request is in partial fulfilment of issue #2019.

Description

Implement asynchronous support in ODataJsonLightParameterWriter

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

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.

pull-request-quantifier-deprecated bot added the Extra Large label

gathogojr requested review from Sreejithpin, ElizabethOkerio, habbes, KenitoInc, marabooy, mikepizzo and xuzhg

May 21, 2021 19:29

xuzhg reviewed

View reviewed changes

src/Microsoft.OData.Core/ODataParameterWriterCore.cs

+                      {
+                          try
+                          {
+                              return await func().ConfigureAwait(false);

Member

xuzhg May 21, 2021

return [](start = 16, length = 6)

return is necessary here?

Contributor Author

gathogojr May 23, 2021

@xuzhg Yes, because we use this overload in scenarios like below where we need the return value:

            ODataWriter resourceWriter = await this.InterceptExceptionAsync(
                () => this.CreateResourceWriterImplementationAsync(parameterName, itemTypeReference)).ConfigureAwait(false);

It's for that same reason why it accepts a parameter of type Func<Task<T>> func - as opposed to Func<Task> for the overload

xuzhg reviewed

View reviewed changes

src/Microsoft.OData.Core/ODataParameterWriterCore.cs Outdated

+                      /// </summary>
+                      /// <param name="action">The delegate to execute asynchronously.</param>
+                      /// <returns>A task that represents the asynchronous operation.</returns>
+                      private async Task InterceptExceptionAsync(Func<Task> action)

Member

xuzhg May 21, 2021

action [](start = 62, length = 6)

here is "action", the below is "func". Maybe need consistent.

xuzhg reviewed

View reviewed changes

src/Microsoft.OData.Core/ODataParameterWriterCore.cs

+                          Debug.Assert(this.State == ParameterWriterState.Start, "this.State == ParameterWriterState.Start");
+                          await this.InterceptExceptionAsync(this.StartPayloadAsync)
+                              .ConfigureAwait(false);

Member

xuzhg May 21, 2021

one line code

Contributor Author

gathogojr May 23, 2021

Below this line we also have:

this.EnterScope(ParameterWriterState.CanWriteParameter);

We await the call to this.InterceptExceptionAsync since we need the this.EnterScope to be executed in the continuation.

xuzhg approved these changes

View reviewed changes

Member

xuzhg left a comment

xuzhg reviewed

View reviewed changes

src/Microsoft.OData.Core/ODataWriterCore.cs Outdated

                       /// <summary>
                       /// Asynchronously start writing a resourceSet.
                       /// </summary>
                       /// <param name="resourceSet">Resource Set/collection to write.</param>
                       /// <returns>A task instance that represents the asynchronous write operation.</returns>
-                      public sealed override Task WriteStartAsync(ODataResourceSet resourceSet)
+                      public sealed override async Task WriteStartAsync(ODataResourceSet resourceSet)

Member

xuzhg May 21, 2021

async [](start = 31, length = 5)

Is it a breaking change?

Contributor Author

gathogojr May 23, 2021

No

KenitoInc reviewed

View reviewed changes

Contributor

KenitoInc left a comment

A few comments

src/Microsoft.OData.Core/JsonLight/ODataJsonLightParameterWriter.cs Show resolved Hide resolved

src/Microsoft.OData.Core/JsonLight/ODataJsonLightParameterWriter.cs Outdated

+                      }
+                      /// <summary>
+              		/// Asynchronously creates a format specific <see cref="ODataWriter"/> to write a resource set.

Contributor

KenitoInc May 24, 2021

check indents

Member

marabooy Jun 9, 2021

@gathogojr Seems like there are mixed tabs and spaces in this file.

src/Microsoft.OData.Core/JsonLight/ODataJsonLightParameterWriter.cs Show resolved Hide resolved

src/Microsoft.OData.Core/JsonLight/ODataJsonLightWriter.cs Outdated Show resolved Hide resolved

src/Microsoft.OData.Core/ODataWriterCore.cs Outdated Show resolved Hide resolved

gathogojr force-pushed the feature/odatajsonlightparameterwriter-async branch from 43c46b3 to 5157ad2 Compare

June 8, 2021 09:00

marabooy approved these changes

View reviewed changes

src/Microsoft.OData.Core/JsonLight/ODataJsonLightParameterWriter.cs Outdated

Comment on lines 189 to 190

		/// <param name="expectedTypeReference">The expected type reference of the parameter value.</param>
		/// <returns>A task that represents the asynchronous write operation.</returns>

Member

marabooy Jun 9, 2021

Suggested change

      
                    /// <param name="expectedTypeReference">The expected type reference of the parameter value.</param>
          
            		/// <returns>A task that represents the asynchronous write operation.</returns>
          
                    /// <param name="expectedTypeReference">The expected type reference of the parameter value.</param>
          
                    /// <returns>A task that represents the asynchronous write operation.</returns>

src/Microsoft.OData.Core/JsonLight/ODataJsonLightParameterWriter.cs Outdated

+                              ODataEnumValue enumVal = parameterValue as ODataEnumValue;
+                              if (enumVal != null)
+                              {

Member

marabooy Jun 9, 2021

Suggested change

      
                            ODataEnumValue enumVal = parameterValue as ODataEnumValue;
          
                            if (enumVal != null)
          
                            {
          
                            if (parameterValue  is  ODataEnumValue enumVal)
          
                            {

src/Microsoft.OData.Core/JsonLight/ODataJsonLightParameterWriter.cs Outdated

+                      }
+                      /// <summary>
+              		/// Asynchronously creates a format specific <see cref="ODataWriter"/> to write a resource set.

Member

marabooy Jun 9, 2021

@gathogojr Seems like there are mixed tabs and spaces in this file.

gathogojr force-pushed the feature/odatajsonlightparameterwriter-async branch 2 times, most recently from 0c0327a to be3e49a Compare

June 9, 2021 14:17

KenitoInc approved these changes

View reviewed changes


          Implement asynchronous support for ODataJsonLightParameterWriter

28cda79

gathogojr force-pushed the feature/odatajsonlightparameterwriter-async branch from be3e49a to 28cda79 Compare

June 11, 2021 07:47

pull-request-quantifier-deprecated bot added Large and removed Extra Large labels

pull-request-quantifier-deprecated bot commented Jun 11, 2021

This PR has 381 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Large
Size       : +347 -34
Percentile : 78.1%

Total files changed: 3

Change summary by file extension:
.cs : +347 -34

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

gathogojr merged commit 819b86c into OData:master

gathogojr deleted the feature/odatajsonlightparameterwriter-async branch

June 11, 2021 08:40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

xuzhg xuzhg approved these changes

marabooy marabooy approved these changes

KenitoInc KenitoInc approved these changes

Sreejithpin Awaiting requested review from Sreejithpin

ElizabethOkerio Awaiting requested review from ElizabethOkerio

habbes Awaiting requested review from habbes

mikepizzo Awaiting requested review from mikepizzo

Labels