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

XmlWriter respects NewLineOnAttributes when writing xmlns #81822

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

devsko
Copy link
Contributor

@devsko devsko commented Feb 8, 2023

Fixes #80306
Fixes #74840

I updated the tt files but did not use them because they are too far out of sync (tried to fix that also but gave up)

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 8, 2023
@ghost
Copy link

ghost commented Feb 8, 2023

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #80306
Fixes #74840

I updated tt files but did not use them because they are too far out of sync (tried to fix that also but gave up)

Author: devsko
Assignees: -
Labels:

area-System.Xml

Milestone: -

await WriteIndentAsync().ConfigureAwait(false);
}

await base.WriteStartNamespaceDeclarationAsync(prefix).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

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

just curious, why not move this logic to base?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I gues you know more about the design decisions of this stuff, but all the indentation is only done in these XxxRawTextWriterIndent classes. I assume to save some bytes when no indentation is needed.

Copy link
Member

Choose a reason for hiding this comment

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

Design was made long time before I joined the team 😄

@krwq

This comment was marked as off-topic.

@azure-pipelines

This comment was marked as off-topic.

@krwq
Copy link
Member

krwq commented Feb 8, 2023

/azp run runtime-libraries-coreclr outerloop-windows

@azure-pipelines

This comment was marked as off-topic.

@krwq
Copy link
Member

krwq commented Feb 8, 2023

Thanks @devsko, overall looks good, just one question and outerloop tests need to run since XML has plenty of them

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.

failures seem to be unrelated

@krwq krwq merged commit c00942f into dotnet:main Feb 9, 2023
@devsko devsko deleted the xmlnewline branch February 9, 2023 18:46
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Xml community-contribution Indicates that the PR has been added by a community member
Projects
None yet
2 participants