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

CI(ruff): Fine tune posting Ruff suggestions #3978

Merged
merged 4 commits into from
Jul 4, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Jul 3, 2024

I overlooked the situation where some ruff errors can and cannot be fixed in the same pass. The exit code won’t be 0 if not all files were fixed. This prevented to reach the step where the diff was stored as an artifact to upload suggestions, to post.

So instead of setting the ruff step with continue-on-errors: true, which would have shown the step as gray (passing), even when rules without fixes are failing, I set the condition to if: ${{ !cancelled() }} as mentioned in the docs for always() https://docs.github.com/en/actions/learn-github-actions/expressions#always

I also spotted a copy-paste typo in the post-pr-reviews workflow, that I had already fixed on another branch but forgot to apply on the one that ended up being a PR.

Finally, I reconsidered my initial position on only applying the safe fixes. Since it is not directly committing, only posting suggestions, it makes sense to show suggestions on what will still remain an error, but without the suggested fixes. Unlike Black, we might need to think a bit about the fixes anyways, when available. Following that thought, I don’t think it’s appropriate in pre-commit to make unsafe fixes on behalf of the user, without any review.

@github-actions github-actions bot added the CI Continuous integration label Jul 3, 2024
@echoix echoix added this to the 8.5.0 milestone Jul 3, 2024
@echoix
Copy link
Member Author

echoix commented Jul 3, 2024

I might at a later point in time work to have full error descriptions in the suggestion too, but it will require a bit more thought to make it work as safely as the current diff-based approach that works fine even from forks

@echoix echoix requested a review from ninsbl July 4, 2024 16:41
Copy link
Member

@ninsbl ninsbl left a comment

Choose a reason for hiding this comment

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

While I cannot judge how this is implemented as I don't know the involved tools to well, your reasoning sounds convincing...

@echoix echoix merged commit 931e79a into OSGeo:main Jul 4, 2024
27 checks passed
@echoix echoix deleted the ruff-suggestions branch July 4, 2024 20:03
a0x8o pushed a commit to a0x8o/grass that referenced this pull request Jul 23, 2024
* CI(ruff): Fix typo in ruff suggestions

* CI(ruff): Upload suggestions even if not all linting errors were fixed

* CI(ruff): Try suggesting more changes

* CI(ruff): Upload suggestions even if not all linting errors were fixed but do not run when cancelled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants