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

Use ASCII strings in source files #78265

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented Nov 12, 2022

This is to align with coding style pt. 16.

Used https://gist.github.com/am11/f35c61783ad7b7c304b3d6e8bd1dcab4 as:

git config core.quotepath off
git ls-files ':/src/*cs' ':/src/*vb' ':/src/*fs' | xargs -I{} conv {} {}

followed by some manual fixes.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 12, 2022
@ghost
Copy link

ghost commented Nov 12, 2022

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

Issue Details

This is to align with coding style pt. 16.

Used https://gist.github.com/am11/f35c61783ad7b7c304b3d6e8bd1dcab4 as:

git config core.quotepath off
git ls-files ':/src/*cs' ':/src/*vb' ':/src/*fs' | xargs -I{} conv {} {}

followed by some manual fixes.

Author: am11
Assignees: -
Labels:

area-Meta, community-contribution

Milestone: -

@danmoseley
Copy link
Member

How do we feel about BOMs - should we remove them all? I guess some would just come back, so it might just be making unnecessary diffs.

@am11
Copy link
Member Author

am11 commented Nov 12, 2022

Yes, there are plenty of files with BOM (1514 files with ':/src/*cs' ':/src/*vb' ':/src/*fs' filter alone). It will create unnecessary diff. For now, I've preserved it. We can remove them in a separate PR. Once sanitized, we can also add a small github workflow for PRs, which checks for its presence in modified files (and fail the job to protect it from regressing).

There are two BOM related changes in this PR delta:

  • XmlSchema.Writer TrimmingTests needed a manual fix with preamble bytes to string, using UTF-8 encoder.
  • TraceSourceConfigurationTests.cs had invalid UTF-8 BOM (I double checked that it wasn't a UTF-16 BOM but first two of three bytes of UTF8 BOM were repeated after the valid BOM).

@am11
Copy link
Member Author

am11 commented Nov 20, 2022

@danmoseley, was there any other comment / concern?

@am11 am11 requested a review from danmoseley November 20, 2022 21:30
@jeffhandley
Copy link
Member

Thanks for the attention to detail on this @am11 (including making author name/email comments consistent). I've spot-checked the characters throughout and all was matching up. Thanks for sharing how you approached this too; that was helpful.

I'm rerunning the failed checks for good measure...

@jeffhandley jeffhandley merged commit 10e929a into dotnet:main Nov 23, 2022
@danmoseley
Copy link
Member

danmoseley commented Nov 23, 2022

Thanks @am11. This definitely has made our tests more robust. The case I was concerned about is tests that have a magic input and end up verifying it matches the output. If those get garbelized probably nobody would have noticed.

ViktorHofer pushed a commit to dotnet/winforms that referenced this pull request Dec 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants