Skip to content
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

Adds IAsyncParseNodeFactory, makes deserialization methods async, upd… #225

Merged
merged 13 commits into from
May 6, 2024
Merged

Adds IAsyncParseNodeFactory, makes deserialization methods async, upd… #225

merged 13 commits into from
May 6, 2024

Conversation

MihaMarkic
Copy link
Contributor

Adds async deserialization capabilities

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting things started, a couple of comments.

Microsoft.Kiota.Abstractions.v3.ncrunchsolution Outdated Show resolved Hide resolved
src/ApiClientBuilder.cs Outdated Show resolved Hide resolved
src/ApiClientBuilder.cs Outdated Show resolved Hide resolved
src/ApiClientBuilder.cs Outdated Show resolved Hide resolved
src/serialization/ParseNodeProxyFactory.cs Outdated Show resolved Hide resolved
src/store/BackingStoreParseNodeFactory.cs Outdated Show resolved Hide resolved
src/serialization/ParseNodeFactoryRegistry.cs Outdated Show resolved Hide resolved
src/serialization/ParseNodeFactoryRegistry.cs Outdated Show resolved Hide resolved
src/serialization/ParseNodeFactoryRegistry.cs Show resolved Hide resolved
src/serialization/ParseNodeProxyFactory.cs Outdated Show resolved Hide resolved
src/serialization/KiotaSerializer.Deserialization.cs Outdated Show resolved Hide resolved
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes, one additional comment besides the obsolete discussion for consistency

src/serialization/ParseNodeFactoryRegistry.cs Outdated Show resolved Hide resolved
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes, a couple of last remarks and we should be good.
Also, can you please:

  1. add a changelog entry (changed, patch, monday)
  2. bump the version (patch)

I think we should preview/validate the changes for the JSON serialization library before we merge this one in, just in case we missed something, so we can validate the design. Thoughts?

baywet
baywet previously approved these changes May 3, 2024
@baywet
Copy link
Member

baywet commented May 3, 2024

@andrueastman for final review and merge!
@MihaMarkic thanks for all the great work! can you start it for text/form/multipart as well please?

andrueastman
andrueastman previously approved these changes May 6, 2024
@andrueastman andrueastman dismissed stale reviews from baywet and themself via bc3f8e1 May 6, 2024 07:22
Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution here @MihaMarkic

@andrueastman andrueastman merged commit 554b2b6 into microsoft:main May 6, 2024
7 checks passed
@MihaMarkic MihaMarkic deleted the feature/asyncdeserialization branch May 10, 2024 13:04
@baywet baywet mentioned this pull request May 22, 2024
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants