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

WIP - Add fantomas formatting support #44801

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

jkone27
Copy link

@jkone27 jkone27 commented Nov 11, 2024

reference issue - dotnet/format#1526

https://learn.microsoft.com/en-us/dotnet/fsharp/style-guide/formatting

https://fsprojects.github.io/fantomas/

not clear how to test this, tried dogfood but is not working ?

Can i test this from some build nuget feed? (similar to this below) does the package build trigger for branches too?

dotnet tool install -g dotnet-format --version "8.0.446201" --add-source https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json

@jkone27 jkone27 requested a review from a team as a code owner November 11, 2024 22:08
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Nov 11, 2024
@jkone27 jkone27 changed the title Add fantomas formatting support WIP - Add fantomas formatting support Nov 11, 2024
Copy link
Contributor

@KalleOlaviNiemitalo KalleOlaviNiemitalo left a comment

Choose a reason for hiding this comment

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

Potential deadlocks with RedirectStandardError, and missing handling for the <PROJECT | SOLUTION> argument.

FileName = "dotnet",
Arguments = "tool list",
RedirectStandardOutput = true,
RedirectStandardError = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

RedirectStandardError = true might cause a deadlock if the child process outputs a lot of text to standard error and the parent process doesn't read from process.StandardError.

Copy link
Author

Choose a reason for hiding this comment

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

shall we switch it off? or what's the best solution for this item?

src/BuiltInTools/dotnet-format/CodeFormatter.cs Outdated Show resolved Hide resolved
src/BuiltInTools/dotnet-format/CodeFormatter.cs Outdated Show resolved Hide resolved
Comment on lines +62 to +65
var isFantomas = await CodeFormatter.IsFantomasInstalled();
if (!isFantomas)
{
CodeFormatter.LogFantomasInstallationInstructions(logger);
Copy link
Contributor

Choose a reason for hiding this comment

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

If something attempts to run dotnet format on F# code, but Fantomas is not installed, then I think it should report an error and return a nonzero exit code, so that it is clear it did not work.

Copy link
Author

@jkone27 jkone27 Nov 18, 2024

Choose a reason for hiding this comment

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

actually many times F# and C# project can be mixed within a single solution, so i wouldnt report an hard error? C# code can still be formatted. in my case i often have e.g. F# unit tests or integ tests on aspnet and maybe few F# modules, but most projects are C#

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm… ok, but if the user specifies the path of a F#-only project without Fantomas installed, then that deserves at least a warning, no?

Copy link
Author

Choose a reason for hiding this comment

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

yes then i should log warning instead of info, right?

@baronfel
Copy link
Member

One proposal I'd like to make is instead of calling dotnet tool list --format json you could use the tool list parsing code directly from the SDK. I'm not entirely sure what value there is in keeping format (and watch, but that's another discussion) outside of the main SDK command tree as a separate executable, and in the past we have had dependency problems due to the separation. @marcpopMSFT do you have opinions on merging the commands more deeply?

@jkone27
Copy link
Author

jkone27 commented Nov 18, 2024

@baronfel i think some stuff is internal atm, so cannot be accessed at this level (for example contracts for dotnet tool list), is there ways i can access tool list from here? is this store registered or available cross dotnet commands?

IEnumerable<IToolPackage> EnumeratePackages();

await process.WaitForExitAsync();

return result is not null
&& result.Data.Any(r => r.PackageId.Contains("fantomas", StringComparison.OrdinalIgnoreCase));
Copy link
Author

Choose a reason for hiding this comment

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

deserializing json output

var output = process.StandardOutput.ReadToEnd();

// log fantomas output
logger.LogInformation(output);
Copy link
Author

Choose a reason for hiding this comment

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

log fantomas output

> dotnet tool install fantomas

""";
logger.LogInformation(message);
Copy link
Author

Choose a reason for hiding this comment

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

updated helper message for fantomas installation instructions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants