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

Remove formatting suppression for tuple syntax #58810

Merged
merged 7 commits into from
Jan 14, 2022
Merged

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jan 12, 2022

Closes #56498 (codestyle for tuple syntax in hiding method was fixed, now updating reference)

@jcouv jcouv marked this pull request as ready for review January 12, 2022 18:44
@jcouv jcouv requested review from a team as code owners January 12, 2022 18:44
@jcouv jcouv requested a review from AlekseyTs January 12, 2022 18:44
@@ -24,7 +24,7 @@
<MicrosoftCodeAnalysisTestingVersion>1.1.1-beta1.21480.3</MicrosoftCodeAnalysisTestingVersion>
<MicrosoftVisualStudioExtensibilityTestingVersion>0.1.122-beta</MicrosoftVisualStudioExtensibilityTestingVersion>
<!-- CodeStyleAnalyzerVersion should we updated together with version of dotnet-format in dotnet-tools.json -->
<CodeStyleAnalyzerVersion>4.0.0-3.final</CodeStyleAnalyzerVersion>
<CodeStyleAnalyzerVersion>4.0.0-6.final</CodeStyleAnalyzerVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Can we use stable version 4.0.1?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have any objection. Any version that includes the formatting fix seems good.
Does 4.0.1 correspond to what's shipped for dev17.0/.NET6 wave?

Copy link
Member

Choose a reason for hiding this comment

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

@JoeRobich or @jmarolf should be able to answer this

Copy link
Member

Choose a reason for hiding this comment

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

Functionally it should be the same, but it likely includes some dependency changes required to build a release package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hitting a problem in correctness leg after new version change. Will take a look, but please let me know if you already have a clue about this:

Unhandled exception: System.IO.FileNotFoundException: Multiple MSBuild solution files found in 'D:\repos\roslyn'. Specify which to use with the <workspace> argument.
   at Microsoft.CodeAnalysis.Tools.Workspaces.MSBuildWorkspaceFinder.FindMatchingFile(String searchBase, Func`2 fileSelector, String multipleFilesFoundError) in /_/src/Workspaces/MSBuildWorkspaceFinder.cs:line 100
   at Microsoft.CodeAnalysis.Tools.Workspaces.MSBuildWorkspaceFinder.FindWorkspace(String searchDirectory, String workspacePath) in /_/src/Workspaces/MSBuildWorkspaceFinder.cs:line 39
   at Microsoft.CodeAnalysis.Tools.Workspaces.MSBuildWorkspaceFinder.FindWorkspace(String searchDirectory, String workspacePath) in /_/src/Workspaces/MSBuildWorkspaceFinder.cs:line 34
   at Microsoft.CodeAnalysis.Tools.FormatCommandCommon.ParseWorkspaceOptions(ParseResult parseResult, FormatOptions formatOptions) in /_/src/Commands/FormatCommandCommon.cs:line 297
   at Microsoft.CodeAnalysis.Tools.Commands.RootFormatCommand.FormatCommandDefaultHandler.InvokeAsync(InvocationContext context) in /_/src/Commands/RootFormatCommand.cs:line 40
   at System.CommandLine.Invocation.InvocationPipeline.<>c__DisplayClass4_0.<<BuildInvocationChain>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass23_0.<<UseParseErrorReporting>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass16_0.<<UseHelp>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass27_0.<<UseVersionOption>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass25_0.<<UseTypoCorrections>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseSuggestDirective>b__24_0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass22_0.<<UseParseDirective>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass11_0.<<UseDebugDirective>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<RegisterWithDotnetSuggest>b__10_0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass14_0.<<UseExceptionHandler>b__0>d.MoveNext()
Command failed to execute with exit code 1: dotnet tool run dotnet-format . --include-generated --include src/Compilers/CSharp/Portable/Generated/ src/Compilers/VisualBasic/Portable/Generated/ src/ExpressionEvaluator/VisualBasic/Source/ResultProvider/Generated/ --check -f
                                                                                                                                                                                                                                                                                System.Management.Automation.RuntimeException: Command failed to execute with exit code 1: dotnet tool run dotnet-format . --include-generated --include src/Compilers/CSharp/Portable/Generated/ src/Compilers/VisualBasic/Portable/Generated/ src/ExpressionEvaluator/VisualBasic/Source/ResultProvider/Generated/ --check -f
                                                                                                                                                                                                                                                                                ```

Copy link
Member

Choose a reason for hiding this comment

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

Surprised this ever worked without specifying either the Roslyn or Compilers solution. We should update the line in test-build-correctness.ps1 replacing the '.' with 'Compilers.sln'.

Exec-Console dotnet "tool run dotnet-format . --include-generated --include src/Compilers/CSharp/Portable/Generated/ src/Compilers/VisualBasic/Portable/Generated/ src/ExpressionEvaluator/VisualBasic/Source/ResultProvider/Generated/ --check -f"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @JoeRobich

Copy link
Member

@JoeRobich JoeRobich Jan 13, 2022

Choose a reason for hiding this comment

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

@jcouv Actually looking at this again, I missed why this broke.

Going from dotnet-format 5.x to 6.x the -f (or --folder) option only applies to the whitespace subcommand. The -f option is why there was no prior issue with too many solutions discovered, because we do not use a msbuild workspace and instead load files directly from disk. I believe the correct arguments would be.

Exec-Console dotnet "tool run dotnet-format whitespace . --folder --include-generated --include src/Compilers/CSharp/Portable/Generated/ src/Compilers/VisualBasic/Portable/Generated/ src/ExpressionEvaluator/VisualBasic/Source/ResultProvider/Generated/ --verify-no-changes"

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 3)

@jcouv jcouv enabled auto-merge (squash) January 14, 2022 00:27
@jcouv jcouv disabled auto-merge January 14, 2022 00:28
Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

The change to the dotnet-format invocation looks good to me. I verified locally that it runs as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected formatting warning for a space between a new modifier and a tuple type syntax.
6 participants