Skip to content

Refactoring XmlReader{Async} - #59337

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

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

Conversation

@kronic

@kronic kronic commented Sep 20, 2021

Copy link
Copy Markdown
Contributor

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

ghost commented Sep 20, 2021

Copy link
Copy Markdown

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
Comment thread src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReader.cs Outdated
scalablecory
scalablecory reviewed Sep 20, 2021
View reviewed changes
Comment thread src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReader.cs Outdated
Comment thread src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReaderAsync.cs Outdated
Comment thread src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReader.cs
krwq
krwq reviewed Sep 21, 2021
View reviewed changes
Comment thread src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReader.cs Outdated
@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

@Wraith2 Wraith2 left a comment

Copy link
Copy Markdown
Contributor

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

scalablecory commented Sep 24, 2021

Copy link
Copy Markdown
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

kronic commented Sep 27, 2021

Copy link
Copy Markdown
Contributor Author

@Wraith2 @scalablecory done

@Wraith2

Wraith2 commented Sep 27, 2021

Copy link
Copy Markdown
Contributor

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

kronic commented Sep 27, 2021

Copy link
Copy Markdown
Contributor Author

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

scalablecory
scalablecory approved these changes Sep 28, 2021
View reviewed changes

@scalablecory scalablecory left a comment

Copy link
Copy Markdown
Contributor

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

kronic commented Oct 4, 2021

Copy link
Copy Markdown
Contributor Author

@krwq ping

krwq
krwq reviewed Oct 5, 2021
View reviewed changes
Comment thread src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReader.cs
krwq
krwq approved these changes Oct 5, 2021
View reviewed changes

@krwq krwq left a comment

Copy link
Copy Markdown
Member

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