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

Added DotNet global tool for code formatting. #29403

Merged
merged 5 commits into from Oct 10, 2018

Conversation

@JoeRobich
Member

JoeRobich commented Aug 20, 2018

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 JoeRobich requested a review from sharwell Aug 20, 2018

@JoeRobich JoeRobich requested review from dotnet/roslyn-ide as code owners Aug 20, 2018

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

This comment has been minimized.

@wli3

wli3 Aug 20, 2018

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.

This comment has been minimized.

@JoeRobich

JoeRobich Aug 21, 2018

Member

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.

.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 comment has been minimized.

@jonsequitur

jonsequitur Aug 20, 2018

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)

This comment has been minimized.

@JoeRobich

JoeRobich Aug 21, 2018

Member

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.

This comment has been minimized.

@jonsequitur

jonsequitur Aug 21, 2018

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 comment has been minimized.

@jonsequitur

jonsequitur Aug 21, 2018

This indeed a bug in the parser.

@JoeRobich JoeRobich requested a review from dotnet/roslyn-compiler as a code owner Aug 21, 2018

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

This comment has been minimized.

@jmarolf

jmarolf Aug 21, 2018

Member

Shouldn't workspacePath be solutionOrProjectPath?

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

This comment has been minimized.

@jmarolf

jmarolf Aug 21, 2018

Member

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.

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

This comment has been minimized.

@jmarolf

jmarolf Aug 21, 2018

Member

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

This comment has been minimized.

@sharwell

sharwell Aug 28, 2018

Member

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.

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

copyright?

else
{
break;
}

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

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

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

Localized message?

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

repeat of constant.

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

repeat of constant.

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

why the need for a timeout?

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

unnecessary indentation.

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));

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

repeat of constant.

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

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

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

this cast seems unnecessary.

@@ -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.

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

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

This comment has been minimized.

@JoeRobich

JoeRobich Aug 22, 2018

Member

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.

<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>

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

"Visual Basic" is two words.

This comment has been minimized.

@JoeRobich

JoeRobich Aug 22, 2018

Member

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

This comment has been minimized.

Contributor

CyrusNajmabadi commented Aug 21, 2018

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

@@ -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;

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

extra blank line.

// 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.

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

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

var formattedDocument = await Formatter.FormatAsync(document, documentOptions, cancellationToken).ConfigureAwait(false);
var documentText = await formattedDocument.GetTextAsync(cancellationToken).ConfigureAwait(false);
solution = solution.WithDocumentText(document.Id, documentText);

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

this is super inefficient. Each time we update a single documeent, we create the new solution snapshot. If anything we do ends up needing semantics, this will cause a ton of work to be done over and over again.

This comment has been minimized.

@jaredpar

jaredpar Aug 21, 2018

Member

Indeed. Ran into this exact problem in the original codeformatter tool. Eventually had to stage the formats into the following:

  1. Syntax only: can do all of these at one time without re-creating a Solution
  2. Semantic which affected one project: allows you to only rebind the projects
  3. Semantic which affected whole solution. No choice but to rebind Solution after every change.

In reply to: 211515699 [](ancestors = 211515699)

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 22, 2018

Contributor

you can also change this to just be: solution = formattedDocument.Project.Solution; The new document already represents a full snapshot of hte world.

This comment has been minimized.

@JoeRobich

JoeRobich Aug 22, 2018

Member

@CyrusNajmabadi I tried the change you suggested and no changes were persisted to disk during the call to .TryApplyChanges(solution). I am not sure what the difference is.

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 22, 2018

Contributor

can you debug through to find out?

private static async Task FormatFilesInSolutionAsync(ILogger logger, Solution solution, string projectPath, CancellationToken cancellationToken)
{
foreach (var projectId in solution.ProjectIds)

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

it seems like we could get a large speedup here by doing things in parallel. even if we processed a single project at a time, we could format all docs in a single project at once, then we could apply all the results at the end of processing that project.

This comment has been minimized.

@jaredpar

jaredpar Aug 21, 2018

Member

I think really only syntax formatting can be done in parallel. Other actions, such as conforming to _ prefix names, can't be done in parallel as edits can cross projects.


In reply to: 211515876 [](ancestors = 211515876)

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

Many semantic changes could not be done in parallel (due to dependencies). However, naming is one of those that can be done in parallel as there aren't actually dependencies (between the edits that is) in that case. i.e. if i have names X, Y and Z, i can rename those all to _X, _Y, _Z at the same time as any edits made from X->_X will be independent (i.e. will only touch a single token) from the other edits.

This comment has been minimized.

@jaredpar

jaredpar Aug 21, 2018

Member

Won't their be dependencies between renames because of shadowing issues? For instance a rename in Project A could require a rename in B to avoid shadowing at the same place B may be doing a rename?


In reply to: 211676254 [](ancestors = 211676254)

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 21, 2018

Contributor

@jaredpar I see. yes, depending on your naming rules, there is the chance you could run into something there.

Though that does seem a bit unlikely... But you're right that it cna't be totally ruled out :-/

@JoeRobich

This comment has been minimized.

Member

JoeRobich commented Sep 13, 2018

retest windows_release_vs-integration_prtest please

@jaredpar

This comment has been minimized.

Member

jaredpar commented Sep 13, 2018

@JoeRobich why are you re-running the VS integration tests? Are they failing due to your change (if so please explain why re-run appropriate). If they're just failing due to flaky then note they are not required to merge.

@JoeRobich

This comment has been minimized.

Member

JoeRobich commented Sep 13, 2018

@jaredpar Ah, will keep that in mind. Guess I was looking for all green even with it being flaky. Thanks

@JoeRobich

This comment has been minimized.

Member

JoeRobich commented Sep 19, 2018

@sharwell could you give this a review?

@JoeRobich JoeRobich changed the base branch from master to dev16.0.x Sep 19, 2018

Added DotNet global tool for 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.
Show resolved Hide resolved src/Tools/dotnet-format/dotnet-format.csproj
Show resolved Hide resolved src/Tools/dotnet-format/README.md Outdated
Show resolved Hide resolved src/Tools/dotnet-format/README.md Outdated
Show resolved Hide resolved src/Tools/dotnet-format/Program.cs Outdated
Show resolved Hide resolved src/Tools/dotnet-format/Program.cs Outdated
Show resolved Hide resolved src/Tools/dotnet-format/MSBuild/CommandUtility.cs Outdated
// 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

This comment has been minimized.

@sharwell

sharwell Sep 19, 2018

Member

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?

This comment has been minimized.

@JoeRobich

JoeRobich Sep 20, 2018

Member

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.

This comment has been minimized.

@DustinCampbell

DustinCampbell Oct 8, 2018

Member

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.

This comment has been minimized.

@nguerrera

nguerrera Oct 9, 2018

Member

There were really two things with the locator PR:

  1. 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.

  2. It ties the locator to implementation details of the CLI. This applies to both approaches. See 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.

Show resolved Hide resolved src/Tools/dotnet-format/MSBuild/MSBuildEnvironment.cs Outdated
Show resolved Hide resolved src/Tools/dotnet-format/MSBuild/MSBuildEnvironment.cs Outdated
Show resolved Hide resolved src/Tools/dotnet-format/MSBuild/MSBuildWorkspaceFinder.cs
@JoeRobich

This comment has been minimized.

Member

JoeRobich commented Sep 20, 2018

@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

This comment has been minimized.

MarcoRossignoli commented Sep 24, 2018

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

This comment has been minimized.

Member

JoeRobich commented Sep 25, 2018

@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

This comment has been minimized.

MarcoRossignoli commented Sep 25, 2018

@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.

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

This comment has been minimized.

@sharwell

sharwell Oct 4, 2018

Member

💡 static class

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

This comment has been minimized.

@sharwell

sharwell Oct 4, 2018

Member

💡 static

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

This comment has been minimized.

@sharwell

sharwell Oct 4, 2018

Member

💡 ImmutableDictionary<LogLevel, ConsoleColor>

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

This comment has been minimized.

@sharwell

sharwell Oct 4, 2018

Member

💡 readonly

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

This comment has been minimized.

@sharwell

sharwell Oct 4, 2018

Member

💡 ImmutableArray<>

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

This comment has been minimized.

@sharwell

sharwell Oct 4, 2018

Member

Why not netcoreapp2.0 like our other projects?

This comment has been minimized.

@tmat

tmat Oct 4, 2018

Member

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

This comment has been minimized.

@wli3

wli3 Oct 5, 2018

global tools only support netcoreapp2.1 and above

This comment has been minimized.

@sharwell

sharwell Oct 9, 2018

Member

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.

This comment has been minimized.

@jaredpar

jaredpar Oct 9, 2018

Member

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

Show resolved Hide resolved ...Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj
@wli3

This comment has been minimized.

wli3 commented Oct 5, 2018

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

This comment has been minimized.

Member

JoeRobich commented Oct 5, 2018

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

@JoeRobich

This comment has been minimized.

Member

JoeRobich commented Oct 8, 2018

Logged ubuntu_16_debug_prtest failure as #30374

@JoeRobich

This comment has been minimized.

Member

JoeRobich commented Oct 8, 2018

retest ubuntu_16_debug_prtest please

@DustinCampbell

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

// 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

This comment has been minimized.

@DustinCampbell

DustinCampbell Oct 8, 2018

Member

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.

Show resolved Hide resolved src/Tools/dotnet-format/MSBuild/MSBuildEnvironment.cs
Show resolved Hide resolved src/Tools/dotnet-format/MSBuild/MSBuildWorkspaceFinder.cs
Show resolved Hide resolved src/Tools/dotnet-format/MSBuild/MSBuildEnvironment.cs Outdated
Show resolved Hide resolved ...Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj
Show resolved Hide resolved 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

nguerrera approved these changes Oct 10, 2018 edited

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 JoeRobich merged commit 0f340a5 into dotnet:dev16.0.x Oct 10, 2018

14 of 16 checks passed

ubuntu_16_mono_debug_prtest Build finished.
Details
windows_debug_vs-integration_prtest Build finished.
Details
WIP ready for review
Details
license/cla All CLA requirements met.
Details
microbuild_prtest Build finished.
Details
ubuntu_16_debug_prtest Build finished.
Details
windows_build_correctness_prtest Build finished.
Details
windows_coreclr_debug_prtest Build finished.
Details
windows_coreclr_release_prtest Build finished.
Details
windows_debug_spanish_unit32_prtest Build finished.
Details
windows_debug_unit32_prtest Build finished.
Details
windows_debug_unit64_prtest Build finished.
Details
windows_determinism_prtest Build finished.
Details
windows_release_unit32_prtest Build finished.
Details
windows_release_unit64_prtest Build finished.
Details
windows_release_vs-integration_prtest Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment