Skip to content

Implement Initializer expression wrappers#39878

Merged
CyrusNajmabadi merged 58 commits intodotnet:mainfrom
tokarzkj:initializer-expression-wrapper
Feb 11, 2022
Merged

Implement Initializer expression wrappers#39878
CyrusNajmabadi merged 58 commits intodotnet:mainfrom
tokarzkj:initializer-expression-wrapper

Conversation

@tokarzkj
Copy link
Copy Markdown
Contributor

@tokarzkj tokarzkj commented Nov 19, 2019 •
edited by sharwell
Loading

Users will now be offered refactoring options for collection initializers. The implementation was modelled after the AbstractSeparatedSyntaxList implementation. This applies to visual basic and c#.

There are 3 choices for users:

  1. Wrap every element
// Before
var example = new[] { 1, 2 };

// After
var example = new[] 
{
    1,
    2
};
  1. Unwrap every element
// Before
var example = new[] 
{
    1,
    2
};

// After
var example = new[] { 1, 2 };
  1. Wrap long
// Before
var example = new[] { 1, 2 };

// After
var example = new[]
{
    1, 2
};

Resolves #37428

tokarzkj added 16 commits November 1, 2019 14:43
@tokarzkj
Add initial unit tests for initializer expression wrapping
4e4da79
@tokarzkj
Setup collection expression wrapper for visual basic
18e990b 
The basic class is in place for visual basic to have the appropriate
wrapping refactoring. The wrapping is mostly correct and just needs
a small tweak for the closing brace to be on a line by itself.
@tokarzkj
Add featured resources for refactorings
fc3fbf5
@tokarzkj
Add the unwrap all refactoring option
8ade65d
@tokarzkj
Add the refactor long initializer line option
05d4b92
@tokarzkj
Get the vb implementation of the refactorings working
ca2ec2d
@tokarzkj
Implement a method to get the spacing for the closing brace
19f2900 
The closing brace is calculated based on what kind of syntax we
are dealing with. If it is a DeclarationKind then we can just align
it based on the span start. If it is a return statement we need to
add an extra tab for spacing because there is no declarationkind for
returns and we passed our starting point.
@tokarzkj
Implement the initializer expression wrapper for csharp
84fa036
@tokarzkj
Get the initializer refactoring working for c#
25665d9
@tokarzkj
Fix the code refactorings for the initializer expression wrapper
928bcec 
- After wrapping if you tried to get refactorings again it was throwing
an exception. Calculating the distance between first node before the {
fixed the issue.

- The unwrap refactoring wasn't moving the opening { to the previous line
@tokarzkj
Fix the long initializer test case that was failing
435202d
@tokarzkj
Get the final TestWrappingShortInitializerTest to work
4783120
@tokarzkj
Add additional tests to confirm the other refactorings are offered
542e210 
These tests confirm that one when one of the refactoring offerings has
been choosen if the user asks for more refactorings they are offered
the other two options to rewrite the initializer.
@tokarzkj
Add Visual Basic tests to confirm the initializer expression wrapper …
95d9314 
…works
@tokarzkj
Rename variables that broke up naming conventions
bed95c2 
Also, remove the old comments that were copied over from the other
wrapping code computer implementations
@tokarzkj
Merge branch 'master' into initializer-expression-wrapper
3eb818e 
Resolve the conflicts in FeatureResources.resx
@tokarzkj tokarzkj marked this pull request as ready for review November 19, 2019 01:53
@tokarzkj tokarzkj requested a review from a team as a code owner November 19, 2019 01:53
CyrusNajmabadi
CyrusNajmabadi reviewed Nov 21, 2019
View reviewed changes
...CSharp/Portable/Wrapping/InitializerExpression/AbstractCSharpInitializerExpressionWrapper.cs Outdated
where TListSyntax : SyntaxNode
where TListItemSyntax : SyntaxNode
{
protected override string Indent_all_items => FeaturesResources.Indent_all_arguments;
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Nov 21, 2019

Choose a reason for hiding this comment

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

"arguments"?

Copy link
Copy Markdown
Contributor Author

@tokarzkj tokarzkj Nov 22, 2019

Choose a reason for hiding this comment

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

Whoops, I thought I changed that. I was going to look at removing these sub labels too unless you think they would be better left in. But, as of now each refactoring only has one option and that seems pointless.

CyrusNajmabadi
CyrusNajmabadi reviewed Nov 21, 2019
View reviewed changes
src/Features/CSharp/Portable/Wrapping/InitializerExpression/CSharpInitializerExpression.cs Outdated

namespace Microsoft.CodeAnalysis.CSharp.Wrapping.InitializerExpression
{
internal partial class CSharpInitializerExpression : AbstractCSharpInitializerExpressionWrapper<InitializerExpressionSyntax, ExpressionSyntax>
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Nov 21, 2019

Choose a reason for hiding this comment

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

CSharpInitializerExpressionWrapper

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jan 27, 2020

@jasonmalinowski i believe this community PR is ready for team review. thanks!

@jasonmalinowski
Copy link
Copy Markdown
Member

jasonmalinowski commented Jan 27, 2020

@CyrusNajmabadi, yep, it's on my list along with seemingly everybody else's. Should get to it in a day or two.

tokarzkj and others added 3 commits January 28, 2020 18:44
@tokarzkj @CyrusNajmabadi
Update src/Features/Core/Portable/Wrapping/AbstractSeparatedListCodeC…
440495c 
…omputer.cs

Co-Authored-By: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
@tokarzkj @CyrusNajmabadi
Apply remove configure await suggestions from code review
98626eb 
Co-Authored-By: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
@tokarzkj @CyrusNajmabadi
Apply additional remove async suggestions from code review
4cfc5d2 
Co-Authored-By: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jan 29, 2020

Thanks @jasonmalinowski !

Base automatically changed from master to main March 3, 2021 23:52
@sharwell
Copy link
Copy Markdown
Contributor

sharwell commented Feb 8, 2022 •
edited
Loading

📝 Note that this feature needs to coordinate with the code style from #54859. Since this PR is much older, it seems like it should merge first and then we update #54859 to make the necessary changes there.

@sharwell
Copy link
Copy Markdown
Contributor

sharwell commented Feb 8, 2022

I'm working on resolving the merge conflicts...

@sharwell sharwell enabled auto-merge February 8, 2022 21:07
@CyrusNajmabadi CyrusNajmabadi force-pushed the initializer-expression-wrapper branch from 425c27b to cb26d8f Compare February 8, 2022 23:25
CyrusNajmabadi
CyrusNajmabadi reviewed Feb 8, 2022
View reviewed changes
...Features/Core/Portable/Wrapping/SeparatedSyntaxList/SeparatedSyntaxListCodeActionComputer.cs Outdated
// MethodName(
// int a, int b, int c, int d, int e, int f, int g, int h, int i, int j)
unwrapActions.Add(await GetUnwrapAllCodeActionAsync(parentTitle, WrappingStyle.WrapFirst_IndentRest).ConfigureAwait(false));
if (this.Wrapper.Supports_UnwrapGroup_WrapFirst_IndentRest)
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Feb 8, 2022

Choose a reason for hiding this comment

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

i recommend reviewing with whitespace off.

@CyrusNajmabadi CyrusNajmabadi force-pushed the initializer-expression-wrapper branch from e4f5b2c to 425c27b Compare February 8, 2022 23:33
@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Feb 8, 2022
Merged
@CyrusNajmabadi CyrusNajmabadi merged commit ef902b7 into dotnet:main Feb 11, 2022
@ghost ghost added this to the Next milestone Feb 11, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022
@Youssef1313 Youssef1313 mentioned this pull request Jun 24, 2022
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

+2 more reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@sharwell sharwell sharwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jasonmalinowski jasonmalinowski

Labels

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

Projects

None yet

Milestone

17.2.P2

Development

Successfully merging this pull request may close these issues.

Wrap array initializer expressions.

6 participants

@tokarzkj @CyrusNajmabadi @jasonmalinowski @sharwell @RikkiGibson @jinujoseph