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.

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

Refactoring XmlReader{Async} #59337

Merged
merged 10 commits into from
Oct 5, 2021
Merged

Refactoring XmlReader{Async} #59337

merged 10 commits into from
Oct 5, 2021

Conversation

kronic
Copy link
Contributor

@kronic kronic commented Sep 20, 2021

No description provided.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 20, 2021
@ghost
Copy link

ghost commented Sep 20, 2021

Tagging subscribers to this area: @buyaa-n, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: kronic
Assignees: -
Labels:

area-System.Xml, community-contribution
Milestone: -

@kronic kronic requested a review from krwq September 22, 2021 22:00
@runfoapp runfoapp bot mentioned this pull request Sep 24, 2021
Closed
@kronic kronic requested a review from Wraith2 September 24, 2021 15:27
Wraith2
Wraith2 suggested changes Sep 24, 2021
View reviewed changes
Copy link
Contributor

@Wraith2 Wraith2 left a comment

Choose a reason for hiding this comment

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

LGTM apart from adding back in the scoped using to make sure the lifetime is explicitly handled correctly as already noted.

@scalablecory
Copy link
Contributor

@kronic I un-resolved some of the comments that appear to not be resolved in current code; can you please get those updated? I agree with @Wraith2 things look fine once those are fixed.

@kronic
Copy link
Contributor Author

kronic commented Sep 27, 2021

@Wraith2 @scalablecory done

@Wraith2
Copy link
Contributor

Wraith2 commented Sep 27, 2021

Per @scalablecory above

The XmlTextWriter needs to be disposed before calling sw.ToString() -- it does flushing and writing of end tags.

The sw.ToString()'s need to be outside/after the using scopes so that flush executes before we try to take the string value.

@kronic
Copy link
Contributor Author

kronic commented Sep 27, 2021

@Wraith2 @scalablecory
sorry, I didn't immediately understand what the problem is.

scalablecory
scalablecory approved these changes Sep 28, 2021
View reviewed changes
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

lgtm; final review to @krwq :)

@kronic
Copy link
Contributor Author

kronic commented Oct 4, 2021

@krwq ping

krwq
krwq approved these changes Oct 5, 2021
View reviewed changes
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

LGTM after resolving comment

@krwq krwq merged commit 854992f into dotnet:main Oct 5, 2021
@kronic kronic deleted the Refactoring_XmlReader{Async} branch October 6, 2021 06:46
@ghost ghost locked as resolved and limited conversation to collaborators Nov 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@krwq krwq krwq approved these changes

@scalablecory scalablecory scalablecory approved these changes

@Wraith2 Wraith2 Wraith2 approved these changes

Assignees

@krwq krwq

Labels
area-System.Xml community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kronic @scalablecory @Wraith2 @krwq