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

Indicate successful check #8553

Closed
bessman opened this issue Nov 8, 2023 · 11 comments
Closed

Indicate successful check #8553

bessman opened this issue Nov 8, 2023 · 11 comments
Labels
cli Related to the command-line interface good first issue Good for newcomers

Comments

@bessman
Copy link

bessman commented Nov 8, 2023

Two of the five testimonials in the README mention intentionally introducing errors to be sure ruff was even running. I admit I did the same when I first tried ruff. It's cool that ruff is so fast people aren't sure it ran at all, but perhaps it is also a UX problem?

When black finds nothing to complain about, it says:

All done! ✨ 🍰 ✨
<n> files left unchanged.

pylint:

Your code has been rated at 10.00/10

mypy:

Success: no issues found in <n> source files

Perhaps ruff could do something similar if no errors are found? For example:

$ ruff check .
Found 0 errors in <n> files 🎉

On the other hand, several linters are silent if they find nothing (pycodestyle, flake8, isort, etc.).

@zanieb
Copy link
Member

zanieb commented Nov 8, 2023

I'm into a message if there are no errors and indicating the number of number of source files sounds nice. I'd prefer not to include an emoji.

@zanieb zanieb added the cli Related to the command-line interface label Nov 8, 2023
@doolio
Copy link
Contributor

doolio commented Nov 9, 2023

On the other hand, several linters are silent if they find nothing (pycodestyle, flake8, isort, etc.).

I presume those that prefer that approach could still achieve it with the --quiet or --silent options.

@zanieb zanieb added the good first issue Good for newcomers label Nov 9, 2023
hughhan1 pushed a commit to hughhan1/ruff that referenced this issue Jan 13, 2024
This implements outputting a message to `stdout` when a check is
run and errors do not occur and fixes are not made.

See astral-sh#8553 for more
information.
hughhan1 pushed a commit to hughhan1/ruff that referenced this issue Jan 13, 2024
This implements outputting a message to `stdout` when a check is
run and errors do not occur and fixes are not made.

See astral-sh#8553 for more
information.
@vladNed
Copy link

vladNed commented Feb 25, 2024

is this issue still available ? I would like to give it a go and implement a nice message output for successful checks.

I got inspired from black, as it always shows the cake when everything was ok

@zanieb
Copy link
Member

zanieb commented Feb 25, 2024

Well it's not done but there are a couple existing pull requests

@hughhan1 opened a pull request but noted that @vinhocent had a better pull request. @konstin reviewed the latter one. I'm not sure what the status is. I'd love to have this done soon though!

@vladNed
Copy link

vladNed commented Feb 26, 2024

I kinda have an idea to also add the checked files to the diagnostics in order to display to the user how many files where checked after a successful run.

Will make a PR and maybe you guys take a look.

@konstin
Copy link
Member

konstin commented Feb 26, 2024

@zanieb Could you review #8631? I can solve the merge conflicts and merge after.

zanieb added a commit that referenced this issue Mar 13, 2024
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

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

<!-- What's the purpose of the change? What does it do, and why? -->

## 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.

<!-- How was it tested? -->

---------

Co-authored-by: Zanie Blue <contact@zanie.dev>
@alexeagle
Copy link

alexeagle commented Mar 27, 2024

I think that users who are trying Ruff for the first time, can't believe the speed and want to see that Ruff is actually doing something are well-advised to introduce a lint violation and see that it's reported, as a way of learning the tool.

#8631 introduced output on success. This has been discussed with other linters:

There are tools which interpret non-empty output to mean that a developers attention is required, and empty output to mean the linter has nothing to report. For example in CI it might report your one-line "success" message as spam boxes in the results UI, where no output would have created no box.

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

@zanieb
Copy link
Member

zanieb commented Mar 27, 2024

Simply, I disagree.

I understand the Unix philosophy and agree with it in many cases, but I don't really see this as a friendly default interface which is an emphasized goal in our tooling.

You should be able to suppress this with -q if it matters for your use-case. I could see automatic suppression when we can't find an interactive terminal being reasonable, but this could also be pretty confusing for people.

I'm willing to hear more discussion and feedback.

There are tools which interpret non-empty output to mean that a developers attention is required, and empty output to mean the linter has nothing to report.

This is the purpose of exit codes. Is there a reason that's insufficient? Deprecation warnings?

@alexeagle
Copy link

I think exit codes don't express warnings well. Non zero exit code would terminate a build, which is an error semantic.

@zanieb
Copy link
Member

zanieb commented Mar 27, 2024

Yeah I think there's an open question about how to elevate warnings to users. Most users don't see any of the deprecation warnings we display in CI and are surprised when things break.

@zanieb
Copy link
Member

zanieb commented Mar 27, 2024

Note we write this to stdout and not stderr (which is where we emit things like warnings)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command-line interface good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

6 participants