Skip to content

Refactoring XmlReader{Async}#59337

Merged
krwq merged 10 commits intodotnet:mainfrom
kronic:Refactoring_XmlReader{Async}
Oct 5, 2021
Merged

Refactoring XmlReader{Async}#59337
krwq merged 10 commits intodotnet:mainfrom
kronic:Refactoring_XmlReader{Async}

Conversation

@kronic
Copy link
Copy Markdown
Contributor

@kronic kronic commented Sep 20, 2021

No description provided.

@ghost ghost added area-System.Xml community-contribution Indicates that the PR has been added by a community member labels Sep 20, 2021
@ghost
Copy link
Copy Markdown

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: -

Wraith2
Wraith2 reviewed Sep 20, 2021
View reviewed changes
scalablecory
scalablecory reviewed Sep 20, 2021
View reviewed changes
krwq
krwq reviewed Sep 21, 2021
View reviewed changes
@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
Copy Markdown
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
Copy Markdown
Contributor

scalablecory commented Sep 24, 2021

@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
Copy Markdown
Contributor Author

kronic commented Sep 27, 2021

@Wraith2 @scalablecory done

@Wraith2
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

kronic commented Oct 4, 2021

@krwq ping

krwq
krwq approved these changes Oct 5, 2021
View reviewed changes
Copy link
Copy Markdown
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

+2 more reviewers

@scalablecory scalablecory scalablecory approved these changes

@Wraith2 Wraith2 Wraith2 approved these changes

Reviewers whose approvals may not affect merge requirements

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.

5 participants

@kronic @scalablecory @Wraith2 @krwq @eiriktsarpalis