Indicated Successful Check

[vinhocent]

Summary

Adds a successful check message after no errors were found 
Implements #8553

Test Plan

Ran a check on a test file with cargo run -p ruff_cli -- check test.py --no-cache and outputted as expected.

Ran the same check with cargo run -p ruff_cli -- check test.py --no-cache --silent and the command was gone as expected.

[vinhocent]

I'm wondering how to add the counting for the number of files checked - I see that it lives in check.rs here but this stat isn't available in diagnostics, is it?

[vinhocent]

Also in the case of tests like this:

Do we even want to output a successful check?

[konstin]

No, we should fail there but currently don't

[nm-remarkable]

This might cause issues with pre-commit since that framework uses "any message at the output" as error. Or bash pre-commit hooks.

[hughhan1]

No, we should fail there but currently don't

@konstin, by "fail", do you specifically mean that we should be exiting with a non-zero exit code? And perhaps continuing to have nothing dumped to stdout?

[konstin]

by "fail", do you specifically mean that we should be exiting with a non-zero exit code?

yes

[zanieb]

@konstin this seems very simple, my only thought is maybe we should have a different message that's in a different format so it's obviously distinct e.g. "No errors found" (I don't love exact message that since it sounds like an error)

@charliermarsh any ideas?

I'd also like to say how many files we scanned e.g. "Found 0 errors in 125 files" would be helpful to know it actually scanned something. We can track that separately.

[konstin]

Slight preference for something like "All checks passed" over something that contains the word error, otherwise i'm on board.

[zanieb]

I like "All checks passed" although it's a little outside our normal terminology

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[zanieb]

I'm not sure what to say here. A configuration option for this seems excessive and it is significantly friendlier for Ruff to indicate that it did something. If you want to open an issue requesting a configuration option, we can consider it. I'd want to see significant community feedback that this is important first though.

[silverwind]

Sorry, I figured out that -q actually works as expected and suppresses this logging just fine, so I deleted my rant. So nothing to do and sorry for the noise :)

[zanieb]

Oh great! :)

[alexeagle]

that framework uses "any message at the output" as error

https://github.com/aspect-build/rules_lint/blob/main/docs/lint_test.md is one such tool. It is used by Bazel users to assert "no warnings were produced by the tool". So this change broke our tests. From what I can tell, ruff is the only linter we integrate that prints on success.

issue requesting a configuration option, we can consider it. I'd want to see significant community feedback that this is important

Here's some other linter decisions on this:

• make the -s, silent output in case of success the default output golangci/golangci-lint#121 - always be silent following https://www.linfo.org/rule_of_silence.html
• eslint default stylish formatter maybe need successful messages output eslint/eslint#3495 (comment) - eslint is incredibly widely adopted and the person commenting here is one of the most prolific and respected developers in the Node.js ecosystem.
• From the (still open?) issue requesting this change, the author observes
  "On the other hand, several linters are silent if they find nothing (pycodestyle, flake8, isort, etc.)."

So I think rather than add an option, ruff should go back to the old silent-on-success behavior.

[MichaReiser]

@alexeagle would you mind creating a new issue for this to avoid having discussions on merged PRs. Thank you

I wonder if it would be sufficient to test if Ruff's in an interactive terminal and only then print the message.

[alexeagle]

@MichaReiser thanks for the quick reply, since the issue requesting this change is still open, I've just commented there: #8553 (comment)

[zanieb]

Thanks for moving the discussion!