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

"Convert to raw string" fixer which removes indentation and initial blank line from the string value #59591

Closed
jnm2 opened this issue Feb 16, 2022 · 13 comments · Fixed by #60936
Labels
Area-IDE Concept-Continuous Improvement Need Design Review The end user experience design needs to be reviewed and approved.
Milestone

Comments

@jnm2
Copy link
Contributor

jnm2 commented Feb 16, 2022

I've commonly chosen to add whitespace to string values where raw string literals would have been perfect, increasing the readability of the code at the expense of actually having unnecessary whitespace in the string value at runtime:

SomeMethod(@"
    intended first line
    intended second line");

Now that raw strings literals are out, I'd like to be able to fix these all up in bulk to become what I would have written originally, had the feature existed:

SomeMethod(
    """
    intended first line
    intended second line
    """);

In the csharplang discussion on this feature, some people have mentioned writing code this way and using an extension method to remove the indentation at runtime. That means this refactoring use case may also exist in places where the indentation would be significant for the string value itself at runtime.

Perhaps the two current Convert to raw string options could be placed into a submenu, and clicking the top-level menu item could perform the first action within the submenu.

image

Then a third item could be added: Convert to raw string > Remove value indentation and initial empty line

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 16, 2022
@AraHaan
Copy link
Member

AraHaan commented Feb 16, 2022

Or anther item could be: Convert raw string to resource. as well, for those who hard code raw strings as well that also need (or well could be) localized.

@jnm2
Copy link
Contributor Author

jnm2 commented Feb 16, 2022

@AraHaan I don't think that's within the scope of this thread. I would also suggest that such a refactoring should not mention what kind of string the literal is; it would work just as well for verbatim strings or normal strings.

@CyrusNajmabadi
Copy link
Member

Or anther item could be: Convert raw string to resource. as well, for those who hard code raw strings as well that also need (or well could be) localized.

Can you open another issue on this. Note: it doesn't really have anything to do with raw-strings. it could apply to all strings.

@CyrusNajmabadi
Copy link
Member

I'm def sympathetic to the use case here. FOr example, we have tons of tests that do this:

await TestAsync(@"
using System;

class Whatever
{
     // etc.
");

That newline exists there just because we don't want to write:

await TestAsync(@"using System;

class Whatever
{
     // etc.
");

But really, the latter is the true code we were trying to test, and the former was written to make it easier to read.

@jinujoseph jinujoseph added Concept-Continuous Improvement Need Design Review The end user experience design needs to be reviewed and approved. and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 23, 2022
@jinujoseph jinujoseph added this to the Backlog milestone Feb 23, 2022
@jinujoseph
Copy link
Contributor

In Design review : To see if this issue is broad enough to invest to bring this item in the refactoring for all users

@sharwell
Copy link
Member

I'm opposed to adding this. All other items preserve the content of strings, but this one will change it. It doesn't seem like a broad enough scenario to warrant a refactoring when manual corrections are so easy.

@jnm2
Copy link
Contributor Author

jnm2 commented Feb 23, 2022

Manual corrections to hundreds of such strings are not feeling easy.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Feb 23, 2022

I'm on the fence here. It's a really interesting case that definitely affects people who often use strings for test cases. I'm extremely biased in that i do that all the time. And i know that this will be a super high pain point for a subet of users. Whether it's important enough for all users is less clear. I wonder if this makes sense to be part of the roslyn-sdk as this really impacts writing tests for code itself.

Will be def to bring to design review to discuss more and get other perspectives on this.

@jnm2
Copy link
Contributor Author

jnm2 commented Feb 23, 2022

For more context: these are SQL commands used with libraries such as Dapper. They often look similar to this, though ranging up over 10 times larger:

            {
                var results = await connection.QueryAsync<(string Name, int Count)>(@"
                    select Name, count(*)
                    from dbo.Things
                    where Type = @type
                    group by Name;",
                    new { type },
                    cancellationToken);

This was a readability compromise driven by the current state of the language, resulting in the following string being sent over the network to SQL Server and appearing in traces:

select Name, count(*)
                    from dbo.Things
                    where Type = @type
                    group by Name;

Now with raw string literals we have the first opportunity to have the best of both worlds, and we have thousands of such strings in multiple projects where we'd like to remove the indentation and first line (if present) from the string content.

@CyrusNajmabadi
Copy link
Member

THanks @jnm2 i think that does show really well the issue that raw-strings was aiming to solve. I personally think it would be unfortunate to add raw strings, but not have some affordance for the above sort of situation whihc is totally reasonable and normal to be in.

@AraHaan
Copy link
Member

AraHaan commented Feb 28, 2022

We could settle with 2 options for “convert to raw string", 1 where indent stays the same and 1 where indent is removed so then it will work for 100% of use cases.

@jnm2
Copy link
Contributor Author

jnm2 commented Feb 28, 2022

@AraHaan That's what I'm asking for, a new option in addition to the current "convert to raw string" options.

@akhera99
Copy link
Member

akhera99 commented Mar 7, 2022

Design Review Conclusion:

We will add a new refactoring that will remove the whitespace at the beginning of the line, at the end of the line, and at the start and end of any new lines.

Perhaps we should also look into adding this as a setting in the editor config, but we will wait and look for feedback about when users feel like this should be enforced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Concept-Continuous Improvement Need Design Review The end user experience design needs to be reviewed and approved.
Projects
Status: Need Update
Development

Successfully merging a pull request may close this issue.

6 participants