belav / csharpier

CSharpier is an opinionated code formatter for c#.
https://csharpier.com
MIT License
1.43k stars 99 forks source link

How to abort bash script if warnings from dotnet CLI tool? #1131

Closed mayka-mack closed 3 months ago

mayka-mack commented 10 months ago

What would be the proper way to abort a bash script if the dotnet CLI tool issues any warnings? I'm going to be formatting files in a CI pipeline during PRs and committing the changes back to the branch, so I do not want the script to move forward if CSharpier returns any warnings during the formatting process.

This is what I came up with initially, but it feels hackish and only works in my regular terminal not my CI pipeline, which seems to still include everything from stdout into the $errors variable, not just warnings and errors.

errors=$(dotnet csharpier . --write-stdout --loglevel Warning 2>&1)

if [ $? -ne 0 ] || [ -n "$errors" ]; then
    echo "$errors"
    echo "CSharpier formatting failed; aborting."
    exit 1
fi

I also tried this to see if any files remained unformatted, but CSharpier still returns exit code 0 with the --check flag when it warns there are files it cannot format.

dotnet csharpier .
dotnet csharpier --check .
if [ $? -ne 0 ]; then
    echo "CSharpier formatting failed; aborting."
    exit 1
fi
belav commented 10 months ago

You'd probably want to ensure everything can be built before running csharpier. It doesn't really care if things compile or not, it just warns you if it can't produce a syntax tree because that means it won't be able to format a given file.

--write-stdout will not actually change any files, it would just write the result of formatting each file to stdout.

You could maybe do things in two passes, with --skip-write the first pass, and if that returns an exit code of 0 then repeat without the flag.

Otherwise if you abort in the middle of formatting then you'll be in a state where only some files are formatted. That may require reverting any changes if you want to continue doing anything in your build.

In my mind the better approach is to just validate the files are formatted on a PR and depend on people running CSharpier. A pre-commit hook can make it essentially automatic, which sounds like what you are trying to do.

mayka-mack commented 10 months ago

You'd probably want to ensure everything can be built before running csharpier. It doesn't really care if things compile or not, it just warns you if it can't produce a syntax tree because that means it won't be able to format a given file.

That is the eventual goal. Currently the pipeline is unable to build the software, since it depends on a proprietary interop package being installed on the build server. While I wait for that to be implemented, I'm primarily using the pipeline just for PR checks.

Otherwise if you abort in the middle of formatting then you'll be in a state where only some files are formatted. That may require reverting any changes if you want to continue doing anything in your build.

CSharpier failing to format all files would imply bigger issues in the project, so I would set up the PR pipeline to fail loudly with a warning and not commit any changes to the repo.

In my mind the better approach is to just validate the files are formatted on a PR and depend on people running CSharpier. A pre-commit hook can make it essentially automatic, which sounds like what you are trying to do.

I'm aiming for something validated on the server side to mandate consistency. I did add the CSharpier.MsBuild nuget package to the solution so that when we build it in VS it will get formatted with CSharpier. The PR pipeline would just be a last line of defense for anything that slips through the cracks.

You could maybe do things in two passes, with --skip-write the first pass, and if that returns an exit code of 0 then repeat without the flag.

Interesting. I tried using --skip-write, but I still received an exit code of 0 when warnings were issued, per the below. Is there a different way I need to be running this to have it give a non-zero exit code when there are formatting warnings?

image

belav commented 10 months ago

I'm aiming for something validated on the server side to mandate consistency. I did add the CSharpier.MsBuild nuget package to the solution so that when we build it in VS it will get formatted with CSharpier. The PR pipeline would just be a last line of defense for anything that slips through the cracks.

By default CSharpier.MsBuild will validate files are formatted when built in release mode. But if you can't actually build the project that is a whole nother problem.

Interesting. I tried using --skip-write, but I still received an exit code of 0 when warnings were issued, per the below. Is there a different way I need to be running this to have it give a non-zero exit code when there are formatting warnings?

CSharpier doesn't consider it an error if a file cannot be formated. It just warns the user that it can't be formatted because it can't compile the file. Actually it parses the file into a syntax tree, maybe I should update that message... anyway. You'd need pipe the output of dotnet csharpier --skip-write to something else and look for the warnings if you want to consider them a failure condition.

rodion-m commented 4 months ago

I also have the same problem when trying to pass this linter into aider. How I have to solve it in a tricky way like:

#!/bin/bash

# Run the dotnet csharpier command and capture its output
output=$(dotnet csharpier "$@")

# Print the output
echo "$output"

# Check if the output contains "Warning" or "error"
if echo "$output" | grep -q -E "Warning|error"; then
    # If it does, exit with a non-zero status code
    exit 1
else
    # If not, exit with the original exit code of dotnet csharpier
    exit ${PIPESTATUS[0]}
fi

@belav Could you please return non-zero exit code when a compilation error occurs?

belav commented 4 months ago

@rodion-m it seems reasonable to return a non-zero exit code for compilation errors. From what I've seen prettier does the same. I thought CSharpier used to do this and was changed on purpose but I can't find any indication of that being true.