intel / p3-analysis-library

A library simplifying the collection and interpretation of P3 data.
https://intel.github.io/p3-analysis-library/
MIT License
7 stars 10 forks source link

[Bug]: Formatting action fails to commit formatting changes #19

Closed Pennycook closed 1 year ago

Pennycook commented 1 year ago

Expected behavior

The current formatting action is intended to perform the formatting and commit the results back.

Actual behavior

When the code in a PR is not correctly formatted, the GitHub action formats it correctly but fails to commit the changes. For example, see https://github.com/intel/p3-analysis-library/actions/runs/5975168848/job/16210629635:

image

Although the action still fails when the code is formatted incorrectly, the error message is confusing to contributors. The log makes it appear as though the formatting was fine, but that something went wrong on our end.

It might be simpler and easier for folks to understand if we remove the auto-commit part of the action. If we just run black --check --diff, the formatting step will fail if code is incorrectly formatted, and the log will clearly show which part of the code needs to be changed, like this:

--- _cascade.py 2023-08-25 17:55:17.222118 +0000
+++ _cascade.py 2023-08-25 17:55:17.837916 +0000
@@ -47,11 +47,12 @@
             lw=1,
         )
         artist.append(rect)

         # Draw text in the box using the platform's assigned label
-        txt = matplotlib.text.Text(            xdescent + width * 0.5,
+        txt = matplotlib.text.Text(
+            xdescent + width * 0.5,
             ydescent + height * 0.4,
             self.labels[name],
             ha="center",
             va="center",
             c="black",
would reformat _cascade.py

Oh no! 💥 💔 💥
1 file would be reformatted.

Steps to reproduce the problem

Open a pull request containing code that is formatted incorrectly.

Specifications

N/A

laserkelvin commented 1 year ago

I deleted my earlier posts because I misread a bunch of things - I still stand by wanting to offload this kind of thing to CI/CD, though, to not burden contributors!

I'll work on a PR to see if I can fix this and if not, we can opt to scale back on the autocommit bit.