Added DotNet global tool for code formatting. #29403

JoeRobich · 2018-08-20T20:21:57Z

Tool can be run on a project, solution, or folder containing a
project or solution. Tool pulls formatting configuration from an
.editorconfig if present, otherwise formats with roslyn defaults.

src/Tools/dotnet-format/dotnet-format.csproj

src/Tools/dotnet-format/Program.cs

wli3 · 2018-08-20T20:53:08Z

src/Tools/dotnet-format/Program.cs

+
+                    MSBuildEnvironment.ApplyEnvironmentVariables();
+
+                    return await CodeFormatter.FormatWorkspaceAsync(logger, workspacePath, isSolution, cancellationTokenSource.Token);


Is this directly calling a fullframework library? Did you try to install the tool and invoke locally? As long as that works it is fine.

I ended up changing the Workspaces.MSBuild project to multi-target. Only required a couple changes to code that was already conditionally compiling. I've tested on Win10 and Ubuntu 18.04.

jonsequitur · 2018-08-20T21:41:25Z

src/Tools/dotnet-format/Program.cs

+                .UseParseErrorReporting()
+                .UseExceptionHandler()
+                .AddOption(new[] { "-w", "--workspace" }, "The project or solution to format", a => a.WithDefaultValue(() => null).ExactlyOne())
+                .AddOption(new[] { "-q", "--quiet" }, "Suppresses all output except warnings and errors", a => a.ParseArgumentsAs<bool>((sr) => ArgumentParseResult.Success(true), new ArgumentArityValidator(0, 0)))


This can be simpler. Just this should do what you want:

a.ParseArgumentsAs<bool>()

The arity is inferred from the type if you don't specify it.

Also, I think the following will always evaluate as true because you're not parsing the sr value that you're receiving in the lambda:

a => a.ParseArgumentsAs<bool>((sr) => ArgumentParseResult.Success(true)

I wanted an arity of 0 for those options since I was treating them as simple flags. This gives the behavior that my tool could be invoked like dotnet format -v .\SomePath\ and verbose would be set and the .\SomePath\ would get interpreted as the workspace. Just using a.ParseArgumentsAs<bool>() would try to interpret .\SomePath\ as a bool and error.

Thankfully it doesn't always return true. It has the desired behavior of only being true when the option is present.

The default behavior for bool is that the argument can be supplied or not (e.g. an arity of 0 - 1), and either way will work:

format -q format -q true format -q false

I'm wondering if you found a bug. I'll try to repro.

This indeed a bug in the parser.

jmarolf · 2018-08-21T06:59:39Z

src/Tools/dotnet-format/CodeFormatter.cs

+{
+    internal class CodeFormatter
+    {
+        public static async Task<int> FormatWorkspaceAsync(ILogger logger, string workspacePath, bool isSolution, CancellationToken token)


Shouldn't workspacePath be solutionOrProjectPath?

jmarolf · 2018-08-21T07:13:11Z

src/Tools/dotnet-format/CodeFormatter.cs

+                { "AlwaysCompileMarkupFilesInSeparateDomain", bool.FalseString },
+
+                // Use the latest language version to force the full set of available analyzers to run on the project.
+                { "LangVersion", "latest" },


I would think we would want to respect the language version of each project. I wouldn't want 7.3 features added if they break my project.

src/Tools/dotnet-format/CodeFormatter.cs

jmarolf · 2018-08-21T07:21:53Z

src/Tools/dotnet-format/CodeFormatter.cs

+                    }
+
+                    var syntaxTree = await document.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false);
+                    if (GeneratedCodeUtilities.IsGeneratedCode(syntaxTree, isCommentTrivia, cancellationToken))


Should we control this with an option? Some people may want to format generated code.

We probably should not. Code generators have the option of applying the formatting themselves during the code generation process, but we should assume that once the tool completes the form is finalized.

src/Tools/dotnet-format/Resources.resx

CyrusNajmabadi · 2018-08-21T08:06:41Z

src/Tools/dotnet-format/Logging/SimpleConsoleLogger.cs

@@ -0,0 +1,66 @@
+using System;


CyrusNajmabadi · 2018-08-21T08:07:06Z

src/Tools/dotnet-format/MSBuild/CommandUtility.cs

+                            else
+                            {
+                                break;
+                            }


all this code looks like an exact duplicate of code that is above.

CyrusNajmabadi · 2018-08-21T08:07:19Z

src/Tools/dotnet-format/MSBuild/CommandUtility.cs

+                                }
+                                catch (Exception ex)
+                                {
+                                    throw new Exception($"Unable to write standard error output when running command {commandInfo.Name}: {ex.Message}");


Localized message?

CyrusNajmabadi · 2018-08-21T08:07:39Z

src/Tools/dotnet-format/MSBuild/CommandUtility.cs

+                        var buffer = new char[512];
+                        while (true)
+                        {
+                            var readCount = process.StandardOutput.Read(buffer, 0, 512);


repeat of constant.

CyrusNajmabadi · 2018-08-21T08:08:13Z

src/Tools/dotnet-format/MSBuild/CommandUtility.cs

+                {
+                    Name = "where",
+                    Arguments = commandName
+                }, timeoutInMilliseconds: 1000);


repeat of constant.

why the need for a timeout?

CyrusNajmabadi · 2018-08-21T08:09:27Z

src/Tools/dotnet-format/MSBuild/MSBuildEnvironment.cs

+        {
+            using (var outputStream = new MemoryStream())
+            {
+                using (var outputStreamWriter = new StreamWriter(outputStream))


unnecessary indentation.

src/Tools/dotnet-format/MSBuild/MSBuildEnvironment.cs

CyrusNajmabadi · 2018-08-21T08:10:15Z

src/Tools/dotnet-format/MSBuild/MSBuildWorkspaceFinder.cs

+        private static IEnumerable<string> FindSolutionFiles(string basePath) => Directory.EnumerateFileSystemEntries(basePath, "*.sln", SearchOption.TopDirectoryOnly);
+
+        private static IEnumerable<string> FindProjectFiles(string basePath) => Directory.EnumerateFileSystemEntries(basePath, "*.*proj", SearchOption.TopDirectoryOnly)
+                    .Where(f => !".xproj".Equals(Path.GetExtension(f), StringComparison.OrdinalIgnoreCase));


repeat of constant.

CyrusNajmabadi · 2018-08-21T08:11:19Z

src/Tools/dotnet-format/Program.cs

+            }
+            catch (Exception ex)
+            {
+                if (ex is TaskCanceledException || ex is OperationCanceledException)


TaskCanceledException is already an OperationCanceledException. So it's redundant to have both checks.

CyrusNajmabadi · 2018-08-21T08:11:31Z

src/Tools/dotnet-format/Program.cs

+                }
+
+                logger.LogError(ex.ToString());
+                logger.LogError((string)"An unexpected error occurred");


this cast seems unnecessary.

CyrusNajmabadi · 2018-08-21T08:11:58Z

src/Tools/dotnet-format/README.md

@@ -0,0 +1,48 @@
+dotnet-format
+=============
+`dotnet-format` is a code formatter for `dotnet` that reads style preferences from your `.editorconfig` file and applies them to a project or solution.


i missed it, but what code actually ensures there is a .editorconfig file?

That is a good point.

In the absence of an .editorconfig file it will use the default formatting options. This behavior might be useful to some users who do not wish to choose formatting options for their project but simply enforce some level of consistency (like Prettier). Something to consider.

CyrusNajmabadi · 2018-08-21T08:12:34Z

src/Tools/dotnet-format/Resources.resx

+    <value>Formatting code files in workspace '{0}'.</value>
+  </data>
+  <data name="Warn_UnsupportedProject" xml:space="preserve">
+    <value>Could not format '{0}'. Format currently supports only C# and VisualBasic projects.</value>


"Visual Basic" is two words.

Thanks. Having a bit of a "Berenstain Bears" moment here. I guess I always assumed in the same way QuickBasic was one word, that Visual Basic was too and just never noticed otherwise.

CyrusNajmabadi · 2018-08-21T08:13:38Z

Dumb question: why does this need to live in the Roslyn repo? It seems like it would be fine living elsewhere.

CyrusNajmabadi · 2018-08-21T08:14:00Z

build/Targets/Packages.props

@@ -268,6 +271,8 @@
      https://myget.org/F/vs-editor/api/v3/index.json;
      https://vside.myget.org/F/vssdk/api/v3/index.json;
      https://vside.myget.org/F/vs-impl/api/v3/index.json;
+      https://dotnet.myget.org/F/system-commandline/api/v3/index.json;
+


extra blank line.

CyrusNajmabadi · 2018-08-21T08:14:53Z

src/Tools/dotnet-format/CodeFormatter.cs

+                // AppDomains will likely not work due to https://github.com/Microsoft/MSBuildLocator/issues/16.
+                { "AlwaysCompileMarkupFilesInSeparateDomain", bool.FalseString },
+
+                // Use the latest language version to force the full set of available analyzers to run on the project.


why do we need analyzers running if this is just updating code formatting?

Tool can be run on a project, solution, or folder containing a project or solution. Tool pulls formatting configuration from an .editorconfig if present, otherwise formats with roslyn defaults.

src/Tools/dotnet-format/dotnet-format.csproj

src/Tools/dotnet-format/README.md

src/Tools/dotnet-format/Program.cs

src/Tools/dotnet-format/MSBuild/CommandUtility.cs

sharwell · 2018-09-19T22:55:25Z

src/Tools/dotnet-format/MSBuild/MSBuildCoreLoader.cs

+
+// LICENSING NOTE: The license for this file is from the originating 
+// source and not the general https://github.com/dotnet/roslyn license.
+// See https://github.com/Microsoft/MSBuildLocator/blob/6631a6dbf9be72b2426e260c99dc0f345e79b8e5/src/MSBuildLocator/MSBuildLocator.cs


❓ What would be the downside if we used MSBuild Locator via PackageReference for now, and sent a pull request to that project to add x-plat support instead of coding it all here in a forked copy?

Looks like they have an open PR to support the .net core use case. I can add a comment that links to the PR and recommends moving to use their package once it gets in.

IIRC, that PR hit some significant roadblocks. Have you reached out to the right folks on the .NET Core SDK and MSBuild teams to determine whether the solution here is the right one? I think the MSBuild Locator PR has pretty much stalled out. At a minimum, I'd start with @nguerrera.

There were really two things with the locator PR:

It doesn't work on Mac (nor all flavors of Linux). This is a technical issue about how it is assuming it can P/Invoke to already loaded hostfxr.dll. This does not apply to the alternate approach of parsing dotnet -info.

It ties the locator to implementation details of the CLI. This applies to both approaches. See Support for locating .NET Core instances microsoft/MSBuildLocator#33 (comment)

As on the locator PR, I can let (2) go here if you agree to update this code if/when the layout changes. We do not consider the current placement of msbuild dlls in the CLI as a contract for tools outside the CLI.

src/Tools/dotnet-format/MSBuild/MSBuildEnvironment.cs

src/Tools/dotnet-format/MSBuild/MSBuildWorkspaceFinder.cs

JoeRobich · 2018-09-20T01:07:38Z

@wli3, Not a lot has changed, but if you can just confirm that things still look OK I would be grateful.

All feedback addressed

MarcoRossignoli · 2018-09-24T08:46:39Z

Is there an ETA for release?
Could be useful to have --verifyonly and --filter, i could use it as CI policy for our devs parsing output or return value.

/cc @JoeRobich @jaredpar

JoeRobich · 2018-09-25T15:50:03Z

@MarcoRossignoli, If the request is to fail a build based on formatting issues, then there is a second PR for a Formatting Analyzer that you will be able to install through NuGet. You could configure the severity of that analyzer to fail your build and give you error locations.

MarcoRossignoli · 2018-09-25T16:06:46Z

@JoeRobich thank's!I'll take a look!

sharwell

The pull request needs to be rewritten to avoid including dotnet-format.2.11.0-dev.nupkg in the source control history.

sharwell · 2018-10-04T17:37:58Z

src/Tools/dotnet-format/CodeFormatter.cs

+
+namespace Microsoft.CodeAnalysis.Tools.CodeFormatter
+{
+    internal class CodeFormatter


💡 static class

sharwell · 2018-10-04T17:38:37Z

src/Tools/dotnet-format/Logging/SimpleConsoleLogger.cs

+        private readonly IConsole _console;
+        private readonly LogLevel _logLevel;
+
+        private readonly IReadOnlyDictionary<LogLevel, ConsoleColor> _logLevelColorMap = new Dictionary<LogLevel, ConsoleColor>


💡 static

sharwell · 2018-10-04T17:39:12Z

src/Tools/dotnet-format/Logging/SimpleConsoleLogger.cs

+        private readonly IConsole _console;
+        private readonly LogLevel _logLevel;
+
+        private readonly IReadOnlyDictionary<LogLevel, ConsoleColor> _logLevelColorMap = new Dictionary<LogLevel, ConsoleColor>


💡 ImmutableDictionary<LogLevel, ConsoleColor>

sharwell · 2018-10-04T17:40:38Z

src/Tools/dotnet-format/Options/EditorConfigOptionsApplier.cs

+{
+    internal class EditorConfigOptionsApplier
+    {
+        private IReadOnlyList<(IOption, OptionStorageLocation, MethodInfo)> _formattingOptionsWithStorage;


💡 readonly

sharwell · 2018-10-04T17:40:55Z

src/Tools/dotnet-format/Options/EditorConfigOptionsApplier.cs

+{
+    internal class EditorConfigOptionsApplier
+    {
+        private IReadOnlyList<(IOption, OptionStorageLocation, MethodInfo)> _formattingOptionsWithStorage;


💡 ImmutableArray<>

sharwell · 2018-10-04T17:41:55Z

src/Tools/dotnet-format/dotnet-format.csproj

+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFramework>netcoreapp2.1</TargetFramework>


❔ Why not netcoreapp2.0 like our other projects?

Yes, should be netcoreapp2.0 in dev16.0.x.

global tools only support netcoreapp2.1 and above

global tools only support netcoreapp2.1 and above

😕 Which part of it? I would expect anything that supports netcoreapp2.1 to also support netcoreapp2.0, and also netcoreapp1.0 for that matter.

Note: we are on netcoreapp2.1 now in master and netcoreapp2.0 is actually no longer even supported.

src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj

wli3 · 2018-10-05T05:11:25Z

For global tools packaging concern, the current state is good. And global tools only support netcoreapp2.1 and up (due to the dependency on the framework dependent apphost added in runtime 2.1)

JoeRobich · 2018-10-05T20:32:30Z

@sharwell I removed the nupkg from the commit history and addressed the open feedback. Could you give it another look?

JoeRobich · 2018-10-08T15:59:02Z

Logged ubuntu_16_debug_prtest failure as #30374

JoeRobich · 2018-10-08T15:59:21Z

retest ubuntu_16_debug_prtest please

DustinCampbell

I think this needs to handle localized dotnet --info output.

DustinCampbell · 2018-10-08T16:36:11Z

src/Tools/dotnet-format/MSBuild/MSBuildCoreLoader.cs

+
+// LICENSING NOTE: The license for this file is from the originating 
+// source and not the general https://github.com/dotnet/roslyn license.
+// See https://github.com/Microsoft/MSBuildLocator/blob/6631a6dbf9be72b2426e260c99dc0f345e79b8e5/src/MSBuildLocator/MSBuildLocator.cs


IIRC, that PR hit some significant roadblocks. Have you reached out to the right folks on the .NET Core SDK and MSBuild teams to determine whether the solution here is the right one? I think the MSBuild Locator PR has pretty much stalled out. At a minimum, I'd start with @nguerrera.

src/Tools/dotnet-format/MSBuild/MSBuildEnvironment.cs

src/Tools/dotnet-format/MSBuild/MSBuildWorkspaceFinder.cs

src/Tools/dotnet-format/MSBuild/MSBuildEnvironment.cs

src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj

THIRD-PARTY-NOTICES.txt

Issues from review now addressed.

DustinCampbell

Looks good! My meta feedback is that we need to get this code into the MSBuildLocator because this PR makes MSBuildWorkspace available to .NET Core applications but those applications still lack the connective tissue to make it work.

nguerrera

I did not review the entire PR, only the place where I was mentioned. I approve this with the understanding that we will look for a more permanent home in MSBuildLocator and also that this approval does not mean that the MSBuild.dll location in the SDK cannot be changed in future versions. The tool here (or better, MSBuildLocator) will need to react to such changes.

JoeRobich requested a review from sharwell August 20, 2018 20:21

JoeRobich requested review from a team as code owners August 20, 2018 20:21

JoeRobich force-pushed the add-formatter-global-tool branch from 317c43d to 9f1894a Compare August 20, 2018 20:23

wli3 reviewed Aug 20, 2018

View reviewed changes

src/Tools/dotnet-format/dotnet-format.csproj Show resolved Hide resolved

wli3 reviewed Aug 20, 2018

View reviewed changes

src/Tools/dotnet-format/Program.cs Show resolved Hide resolved

wli3 reviewed Aug 20, 2018

View reviewed changes

jonsequitur reviewed Aug 20, 2018

View reviewed changes

JoeRobich force-pushed the add-formatter-global-tool branch from 9f1894a to 9f69e72 Compare August 21, 2018 00:05

JoeRobich requested a review from a team as a code owner August 21, 2018 01:08

jmarolf reviewed Aug 21, 2018

View reviewed changes

src/Tools/dotnet-format/CodeFormatter.cs Show resolved Hide resolved

jmarolf reviewed Aug 21, 2018

View reviewed changes

src/Tools/dotnet-format/Resources.resx Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 21, 2018

View reviewed changes

src/Tools/dotnet-format/Logging/SimpleConsoleLogger.cs Outdated

@@ -0,0 +1,66 @@

using System;

Copy link

Member

CyrusNajmabadi Aug 21, 2018

Choose a reason for hiding this comment

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

copyright?

CyrusNajmabadi reviewed Aug 21, 2018

View reviewed changes

src/Tools/dotnet-format/MSBuild/MSBuildEnvironment.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 21, 2018

View reviewed changes

JoeRobich force-pushed the add-formatter-global-tool branch from a989e11 to 09900cf Compare September 19, 2018 21:32

JoeRobich changed the base branch from master to dev16.0.x September 19, 2018 21:36

JoeRobich force-pushed the add-formatter-global-tool branch from 09900cf to 0bcddd8 Compare September 19, 2018 21:39

Added DotNet global tool for code formatting.

6113fa0

Tool can be run on a project, solution, or folder containing a project or solution. Tool pulls formatting configuration from an .editorconfig if present, otherwise formats with roslyn defaults.

JoeRobich force-pushed the add-formatter-global-tool branch from 0bcddd8 to 6113fa0 Compare September 19, 2018 22:13

sharwell previously requested changes Sep 19, 2018

View reviewed changes

sharwell previously requested changes Oct 4, 2018

View reviewed changes

JoeRobich added 3 commits October 5, 2018 13:29

Incorporated feedback from PR

a40162e

Updated licensing

c04f675

Incorporated feedback from code review

8571fde

JoeRobich force-pushed the add-formatter-global-tool branch from d9ae946 to 8571fde Compare October 5, 2018 20:30

DustinCampbell requested changes Oct 8, 2018

View reviewed changes

Implement feedback from PR

3207024

DustinCampbell approved these changes Oct 9, 2018

View reviewed changes

sharwell approved these changes Oct 9, 2018

View reviewed changes

nguerrera approved these changes Oct 10, 2018

View reviewed changes

jinujoseph added the Approved to merge label Oct 10, 2018

JoeRobich merged commit 0f340a5 into dotnet:dev16.0.x Oct 10, 2018

JoeRobich mentioned this pull request Oct 16, 2018

Added AnalyzerRunner flag to Format all code files in solution #29187

Closed

JoeRobich deleted the add-formatter-global-tool branch January 14, 2019 23:52


		MSBuildEnvironment.ApplyEnvironmentVariables();

		return await CodeFormatter.FormatWorkspaceAsync(logger, workspacePath, isSolution, cancellationTokenSource.Token);

Added DotNet global tool for code formatting. #29403

Added DotNet global tool for code formatting. #29403

Conversation

JoeRobich commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonsequitur Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarolf Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeRobich commented Sep 20, 2018 • edited Loading

MarcoRossignoli commented Sep 24, 2018 • edited Loading

JoeRobich commented Sep 25, 2018

MarcoRossignoli commented Sep 25, 2018

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 commented Oct 5, 2018

JoeRobich commented Oct 5, 2018

JoeRobich commented Oct 8, 2018

JoeRobich commented Oct 8, 2018

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

nguerrera left a comment • edited Loading

Choose a reason for hiding this comment

jonsequitur Aug 20, 2018 •

edited

Loading

jmarolf Aug 21, 2018 •

edited

Loading

sharwell Aug 28, 2018 •

edited

Loading

JoeRobich commented Sep 20, 2018 •

edited

Loading

MarcoRossignoli commented Sep 24, 2018 •

edited

Loading

sharwell Oct 9, 2018 •

edited

Loading

nguerrera left a comment •

edited

Loading