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

feat: enable jumping to "not formatted" files from VisualStudio error list #1517

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions Src/CSharpier.Cli/CommandLineFormatter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ CancellationToken cancellationToken
{
var fileIssueLogger = new FileIssueLogger(
commandLineOptions.OriginalDirectoryOrFilePaths[0],
logger
logger,
commandLineOptions.MsBuildFormat
);

var printerOptions = optionsProvider.GetPrinterOptionsFor(filePath);
Expand Down Expand Up @@ -231,7 +232,11 @@ await FormatPhysicalFile(
}
else if (warnForUnsupported)
{
var fileIssueLogger = new FileIssueLogger(originalFilePath, logger);
var fileIssueLogger = new FileIssueLogger(
originalFilePath,
logger,
isMsBuildFormat: false
);
fileIssueLogger.WriteWarning("Is an unsupported file type.");
}
}
Expand Down Expand Up @@ -307,7 +312,11 @@ CancellationToken cancellationToken
cancellationToken
);

var fileIssueLogger = new FileIssueLogger(originalFilePath, logger);
var fileIssueLogger = new FileIssueLogger(
originalFilePath,
logger,
commandLineOptions.MsBuildFormat
);

logger.LogDebug(
commandLineOptions.Check
Expand Down
6 changes: 6 additions & 0 deletions Src/CSharpier.Cli/CommandLineOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ internal class CommandLineOptions
{
public string[] DirectoryOrFilePaths { get; init; } = [];
public bool Check { get; init; }
public bool MsBuildFormat { get; init; }
public bool Fast { get; init; }
public bool SkipWrite { get; init; }
public bool WriteStdout { get; init; }
Expand All @@ -21,6 +22,7 @@ internal class CommandLineOptions
internal delegate Task<int> Handler(
string[] directoryOrFile,
bool check,
bool msBuildFormat,
bool fast,
bool skipWrite,
bool writeStdout,
Expand Down Expand Up @@ -52,6 +54,10 @@ public static RootCommand Create()
() => LogLevel.Information.ToString(),
"Specify the log level - Debug, Information (default), Warning, Error, None"
),
new Option(
["--msbuild-format"],
"Formats messages in standard error/warning format for MSBuild."
),
Copy link
Owner

Choose a reason for hiding this comment

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

There have been requests for other log formats. I'm thinking this should be --log-format and accept a string/enum. Just msbuild and default (or maybe call it console?) for now.

Copy link
Author

Choose a reason for hiding this comment

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

Renamed Startparameter and used enum as suggested.

new Option(
["--no-cache"],
"Bypass the cache to determine if a file needs to be formatted."
Expand Down
5 changes: 3 additions & 2 deletions Src/CSharpier.Cli/ConsoleLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

namespace CSharpier.Cli;

internal class ConsoleLogger(IConsole console, LogLevel loggingLevel) : ILogger
internal class ConsoleLogger(IConsole console, LogLevel loggingLevel, bool isMsBuildFormat)
: ILogger
{
private static readonly object ConsoleLock = new();

Expand Down Expand Up @@ -52,7 +53,7 @@ void WriteLine(string? value = null)
{
var message = formatter(state, exception!);

if (logLevel >= LogLevel.Warning)
if (!isMsBuildFormat && logLevel >= LogLevel.Warning)
{
console.ForegroundColor = GetColorLevel(logLevel);
Write($"{logLevel} ");
Expand Down
7 changes: 5 additions & 2 deletions Src/CSharpier.Cli/FileIssueLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@

namespace CSharpier.Cli;

internal class FileIssueLogger(string filePath, ILogger logger)
internal class FileIssueLogger(string filePath, ILogger logger, bool isMsBuildFormat)
{
public void WriteError(string value, Exception? exception = null)
{
logger.LogError(exception, $"{this.GetPath()} - {value}");
if (isMsBuildFormat)
logger.LogError("{Path}: error: {Message}", this.GetPath(), value);
Copy link
Owner

Choose a reason for hiding this comment

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

I think the WriteWarning message should also use msbuild format. Although I don't recall what is considered a warning by CSharpier, and we turn on warningsAsErrors at work. Maybe that would cause issues.

Copy link
Author

Choose a reason for hiding this comment

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

Also made WriteWarning() obey the --log-format parameter.
Also added a TODO comment for adding a test as soon as it is clear in which situations CSharpier will output warnings.

else
logger.LogError(exception, $"{this.GetPath()} - {value}");
}

public void WriteWarning(string value)
Expand Down
4 changes: 3 additions & 1 deletion Src/CSharpier.Cli/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public static async Task<int> Main(string[] args)
public static async Task<int> Run(
string[]? directoryOrFile,
bool check,
bool msBuildFormat,
bool fast,
bool skipWrite,
bool writeStdout,
Expand All @@ -42,7 +43,7 @@ CancellationToken cancellationToken
var actualConfigPath = string.IsNullOrEmpty(configPath) ? null : configPath;

var console = new SystemConsole();
var logger = new ConsoleLogger(console, logLevel);
var logger = new ConsoleLogger(console, logLevel, msBuildFormat);

if (pipeMultipleFiles)
{
Expand Down Expand Up @@ -89,6 +90,7 @@ CancellationToken cancellationToken
OriginalDirectoryOrFilePaths = originalDirectoryOrFile!,
StandardInFileContents = standardInFileContents,
Check = check,
MsBuildFormat = msBuildFormat,
NoCache = noCache,
NoMSBuildCheck = noMSBuildCheck,
Fast = fast,
Expand Down
4 changes: 1 addition & 3 deletions Src/CSharpier.MsBuild/build/CSharpier.MsBuild.targets
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,7 @@
StdOutEncoding="utf-8"
StdErrEncoding="utf-8"
IgnoreExitCode="true"
IgnoreStandardErrorWarningFormat="true"
CustomErrorRegularExpression="^Error "
Command="&quot;$(CSharpier_dotnet_Path)&quot; exec &quot;$(CSharpierDllPath)&quot; $(CSharpierArgs) --no-msbuild-check --compilation-errors-as-warnings &quot;$(MSBuildProjectDirectory)&quot; &gt; $(NullOutput) "
Command="&quot;$(CSharpier_dotnet_Path)&quot; exec &quot;$(CSharpierDllPath)&quot; $(CSharpierArgs) --msbuild-format --no-msbuild-check --compilation-errors-as-warnings &quot;$(MSBuildProjectDirectory)&quot; &gt; $(NullOutput) "
/>
</Target>
<!-- getting this to run a single time for projects that target multiple frameworks requires all of this
Expand Down
28 changes: 25 additions & 3 deletions Src/CSharpier.Tests/CommandLineFormatterTests.cs
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
using System.IO.Abstractions.TestingHelpers;
using System.Text;
using CSharpier.Cli;
using FluentAssertions;
using Microsoft.Extensions.Logging;
using NUnit.Framework;
using System.IO.Abstractions.TestingHelpers;
using System.Text;

namespace CSharpier.Tests;

[TestFixture]
[Parallelizable(ParallelScope.All)]
public class CommandLineFormatterTests
{
private const bool IsMsBuildFormat = true;
private const bool IsNotMsBuildFormat = false;
Copy link
Owner

Choose a reason for hiding this comment

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

These don't appear to be used and should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

Removed leftovers :)


private const string UnformattedClassContent = "public class ClassName { public int Field; }";
private const string FormattedClassContent =
"public class ClassName\n{\n public int Field;\n}\n";
Expand Down Expand Up @@ -362,6 +365,23 @@ public void Format_Checks_Unformatted_File()
.StartWith("Error ./Unformatted.cs - Was not formatted.");
}

[Test]
public void Format_Checks_Unformatted_File_With_MsBuildFormat_Message()
{
var context = new TestContext();
const string unformattedFilePath = "Unformatted.cs";
context.WhenAFileExists(unformattedFilePath, UnformattedClassContent);

var result = Format(context, check: true, msBuildFormat: true);

result.ExitCode.Should().Be(1);
context.GetFileContent(unformattedFilePath).Should().Be(UnformattedClassContent);
result
.ErrorOutputLines.First()
.Should()
.StartWith("./Unformatted.cs: error: Was not formatted.");
}

[TestCase("Src/node_modules/File.cs")]
[TestCase("node_modules/File.cs")]
[TestCase("node_modules/Folder/File.cs")]
Expand Down Expand Up @@ -750,6 +770,7 @@ private static FormatResult Format(
TestContext context,
bool skipWrite = false,
bool check = false,
bool msBuildFormat = false,
bool writeStdout = false,
bool includeGenerated = false,
bool compilationErrorsAsWarnings = false,
Expand All @@ -772,7 +793,7 @@ params string[] directoryOrFilePaths
}

var fakeConsole = new TestConsole();
var testLogger = new ConsoleLogger(fakeConsole, LogLevel.Information);
var testLogger = new ConsoleLogger(fakeConsole, LogLevel.Information, msBuildFormat);
var exitCode = CommandLineFormatter
.Format(
new CommandLineOptions
Expand All @@ -782,6 +803,7 @@ params string[] directoryOrFilePaths
OriginalDirectoryOrFilePaths = originalDirectoryOrFilePaths,
SkipWrite = skipWrite,
Check = check,
MsBuildFormat = msBuildFormat,
WriteStdout = writeStdout || standardInFileContents != null,
StandardInFileContents = standardInFileContents,
IncludeGenerated = includeGenerated,
Expand Down