Skip to content

Add feature to simplify interpolation expressions when possible #40070

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 48 commits into from
Dec 18, 2019
Merged

Add feature to simplify interpolation expressions when possible #40070

merged 48 commits into from
Dec 18, 2019

Conversation

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Nov 30, 2019 •
edited
Loading

Fixes #40066

Looks like this:

image

Todo:

  • vb
  • tests

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner November 30, 2019 02:58
@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Nov 30, 2019

Tagging @jinujoseph @dotnet/roslyn-ide

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Nov 30, 2019

@jnm2 Would you be willing to help with tests?

@jnm2
Copy link
Contributor

jnm2 commented Nov 30, 2019

@CyrusNajmabadi Sure, soon as I can.

jnm2
jnm2 reviewed Dec 1, 2019
View reviewed changes
jnm2
jnm2 reviewed Dec 1, 2019
View reviewed changes
src/Features/Core/Portable/SimplifyInterpolation/Helpers.cs
Comment on lines 63 to 76
if (invocation.Arguments.Length == 1 &&
invocation.Arguments[0].Value.ConstantValue is { HasValue: true, Value: string format })
{
unwrapped = invocation.Instance;
formatString = format;
return;
}

if (invocation.Arguments.Length == 0)
{
unwrapped = invocation.Instance;
formatString = "";
return;
}
Copy link
Contributor

@jnm2 jnm2 Dec 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Option:

if (invocation.Arguments.Length <= 1)
{
    unwrapped = invocation.Instance;

    formatString = invocation.Arguments.FirstOrDefault()?.Value.ConstantValue.GetValueOrDefault()
        ?? string.Empty;
}

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 6, 2019

Choose a reason for hiding this comment

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

i'm ok in the current form. seems clearer to me.

@jnm2
Copy link
Contributor

jnm2 commented Dec 1, 2019

@CyrusNajmabadi What's the best way to help with tests? PR to this branch in your fork?

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 2, 2019

PR to this branch in your fork?

yes please!

@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature Request labels Dec 2, 2019
jnm2
jnm2 reviewed Dec 7, 2019
View reviewed changes
@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 9, 2019

@CyrusNajmabadi I gave you failing tests on purpose (like I said in chat) and it doesn't look like you changed the feature to pass them yet.

Oh, ok. I'll take a look :)

@jnm2
Copy link
Contributor

jnm2 commented Dec 9, 2019 •
edited
Loading

Did you want me to do make the feature changes too? A couple of tests are suggestions (treat them all that way) and you mentioned wanting to be the one to merge the consecutive Unnecessary spans before comparing them to the test markup.

@jnm2
Copy link
Contributor

jnm2 commented Dec 9, 2019

Ah 👍

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 12, 2019

@ryzngard LMK what you need here. This is ready for review.

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 16, 2019

@jnm2 thanks for all your help!
@ryzngard plz review when you get a chance. Thanks!

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 16, 2019

Hey @ryzngard could you ptal at this PR? It's been more than two weeks without any movement... :-/

ryzngard
ryzngard reviewed Dec 18, 2019
View reviewed changes
src/EditorFeatures/CSharpTest/SimplifyInterpolation/SimplifyInterpolationTests.cs
[Fact]
public async Task ToStringWithNoParameterWhenBothComponentsAreSpecified()
{
await TestMissingInRegularAndScriptAsync(
Copy link
Contributor

@ryzngard ryzngard Dec 17, 2019

Choose a reason for hiding this comment

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

We should support simplifying this scenario as well. Will file a separate task

Copy link
Contributor

@jnm2 jnm2 Dec 18, 2019

Choose a reason for hiding this comment

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

@ryzngard I wrote the test this way because I thought of it as a correctness issue. The goo format string is being applied to the return value of ToString() and is a no-op. Removing ToString() would cause it to stop being a no-op. Wouldn't it be better to put a warning squiggle on :goo than to mark ToString() as redundant?

Copy link
Contributor

@jnm2 jnm2 Dec 18, 2019

Choose a reason for hiding this comment

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

That would be a separate feature though, a warning squiggle whenever it is known that the format component cannot be applied to the expression type.

@ryzngard ryzngard merged commit 2b8a9f7 into dotnet:master Dec 18, 2019
@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 18, 2019

Thanks!

@CyrusNajmabadi CyrusNajmabadi deleted the simplifyInterpolation branch December 18, 2019 21:59
@RehanSaeed RehanSaeed mentioned this pull request Feb 10, 2020
Merged
jnm2
jnm2 reviewed Feb 21, 2020
View reviewed changes
src/Features/Core/Portable/SimplifyInterpolation/Helpers.cs

unwrapped = invocation.Instance;
alignment = alignmentSyntax as TExpressionSyntax;
negate = targetName == nameof(string.PadLeft);
Copy link
Contributor

@jnm2 jnm2 Feb 21, 2020

Choose a reason for hiding this comment

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

😱 We did a bad thing. Fix in #41845.

@jnm2 jnm2 mentioned this pull request Feb 21, 2020
Closed
@sharwell sharwell added this to the 16.5.P2 milestone Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@jnm2 jnm2 jnm2 left review comments

@ryzngard ryzngard ryzngard approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ryzngard ryzngard

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature Request

Projects

None yet

Milestone

16.5.P2

Development

Successfully merging this pull request may close these issues.

Suggestion to replace ToString at end of interpolation expression with colon and format code

5 participants

@CyrusNajmabadi @jnm2 @ryzngard @sharwell @jinujoseph