Skip to content

Convert concatenation string to interpolation for System.Private.Xml #60057

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
Merged
merged 17 commits into from
Nov 9, 2021
Merged

Convert concatenation string to interpolation for System.Private.Xml #60057

merged 17 commits into from
Nov 9, 2021

Conversation

kronic
Copy link
Contributor

@kronic kronic commented Oct 6, 2021

No description provided.

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

ghost commented Oct 6, 2021

Tagging subscribers to this area: @dotnet/area-system-xml
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: -

krwq
krwq reviewed Oct 6, 2021
View reviewed changes
@kronic kronic requested a review from krwq October 6, 2021 09:14
@runfoapp runfoapp bot mentioned this pull request Oct 6, 2021
Closed
@kronic kronic closed this Oct 18, 2021
@eiriktsarpalis
Copy link
Member

Hi @kronic, curious why the PR was closed?

@kronic kronic reopened this Oct 18, 2021
@kronic
Copy link
Contributor Author

kronic commented Oct 18, 2021 •
edited
Loading

@eiriktsarpalis Hello, closed by mistake

eiriktsarpalis
eiriktsarpalis approved these changes Oct 19, 2021
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

LGTM although might need another pair of eyes.

@runfoapp runfoapp bot mentioned this pull request Oct 20, 2021
Closed
@kronic
Copy link
Contributor Author

kronic commented Oct 27, 2021

ping @eiriktsarpalis @krwq

stephentoub
stephentoub reviewed Nov 5, 2021
View reviewed changes
src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextWriter.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return $"d{_top.ToString("d", CultureInfo.InvariantCulture)}p{temp.ToString("d", CultureInfo.InvariantCulture)}";
return string.Create(CultureInfo.InvariantCulture, $"d{_top}p{temp}";

Copy link
Contributor Author

@kronic kronic Nov 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@stephentoub maybe

string.Create(CultureInfo.InvariantCulture, $"d{_top:d}p{temp:d}";

@kronic
Copy link
Contributor Author

kronic commented Nov 8, 2021

@stephentoub ping

@eiriktsarpalis eiriktsarpalis force-pushed the Convert_concatenation_to_interpolation branch from a3a0ec8 to 82feb68 Compare November 8, 2021 12:56
@eiriktsarpalis
Copy link
Member

Thank you @kronic!

@eiriktsarpalis eiriktsarpalis merged commit 08aff10 into dotnet:main Nov 9, 2021
@kronic kronic deleted the Convert_concatenation_to_interpolation branch November 9, 2021 11:40
This was referenced Nov 29, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub approved these changes

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

@krwq krwq Awaiting requested review from krwq

+1 more reviewer

@am11 am11 am11 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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 @eiriktsarpalis @krwq @stephentoub @am11