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

Add refactoring to convert normal (or verbatim) strings to raw strings. #59180

Merged
merged 14 commits into from
Feb 2, 2022

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Jan 31, 2022

Looks like this:

image

Workign on the interpolated form now.

Relates to test plan #55306

@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners January 31, 2022 20:32
@CyrusNajmabadi CyrusNajmabadi changed the base branch from main to features/RawStringLiterals January 31, 2022 20:38
// Check if we have an escaped character in the original string.
if (ch.Span.Length > 1)
{
// An escaped newline is fine to convert (to a multi-line raw string).
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a very Windows-centric assumption. What characters does IsNewLine match? Because, and maybe I missed them, but I didn't see tests for other newline characters like \n and \r on their own.

I think this needs a (may change semantics) (or some other wording) note, because this converts specifically defined newline character sequences into whatever the current machine/git repo/etc. happens to be configured to use. If a user checks out a git repo on a mac, runs this refactoring on VS Mac, this could break things.

Unless I'm wrong and the new raw strings exclusively encode \r\n into the value? If so, then I guess this is okay, but would be good to confirm that IsNewLine doesn't match anything other than that.

Copy link
Member Author

Choose a reason for hiding this comment

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

So this is a good point. For clarity, whatever newline sequence you wrote, we will embed into your converted string. This means from the language/compiler perspective, on that machine, the literal has the exact meaning as before (no semantics change).

Where it gets subtle though is the absolutely insane behavior of git where it may take a file and just change newlines even that are language relevant when sent to another machine.

In this case, the refactoring is in line with the lang and compiler. However, it is git that ends up causing issues here as it thinks it is sensible to touch programming lines that relate to content.

--

Note that this is a refactoring though. It is opted into the user, presumably because they actually think this is better. And it will be very clear that a single line became multi-line. So i think the user can be trusted to decide if that is acceptable for them given their git config.

Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, whatever newline sequence you wrote, we will embed into your converted string. This means from the language/compiler perspective, on that machine, the literal has the exact meaning as before (no semantics change).

Read the code now, and yes, I can see that. I think thats probably good, and perhaps even the saviour of it. If I was on a system that used LF, and converted a string that had CRLF, I would expect the IDE would somehow alert me to the fact that I had mixed line endings, and I could easily Ctrl+Z.

@sharwell sharwell changed the title Add fixer to convert normal (or verbatim) strings to raw strings. Add refactoring to convert normal (or verbatim) strings to raw strings. Feb 1, 2022
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 2, 2022 18:18
@CyrusNajmabadi CyrusNajmabadi changed the base branch from features/RawStringLiterals to main February 2, 2022 18:18
@CyrusNajmabadi CyrusNajmabadi merged commit aa97465 into dotnet:main Feb 2, 2022
@ghost ghost added this to the Next milestone Feb 2, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the rawStringAnalyzerFixer branch February 18, 2022 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants