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

Closed
wants to merge 1 commit into from

Conversation

hughhan1
Copy link

Summary

This implements outputting a message to stdout when a check is run and errors do not occur and fixes are not made. This is done only for when watch mode is not enabled; running via watch mode will continue to not output anything when errors do not occur and fixes are not made.

See #8553 for more information.

Test Plan

Snapshot tests.

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.
noqa.into(),
fix_mode,
)?,
1,
Copy link
Author

Choose a reason for hiding this comment

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

Flagging here that I'm returning 1 here for the number of affected files if we're reading from stdin.

Please let me know if this is okay!

Comment on lines 1107 to 1111
----- stdout -----
Found 0 errors in 1 file

----- stderr -----
warning: Encountered error: Permission denied (os error 13)
Copy link
Author

Choose a reason for hiding this comment

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

Flagging here that we're displaying the Found 0 errors in 1 file, even when we run into a permission-denied error. Perhaps the output here is undesirable?

Open to feedback and suggestions on what we think the ideal product behavior should look like!

Comment on lines +433 to +436
if checked_files > 0 && diagnostics.fixed.is_empty() && diagnostics.messages.is_empty() {
let s = if checked_files == 1 { "" } else { "s" };
writeln!(summary_writer, "Found 0 errors in {checked_files} file{s}")?;
}
Copy link
Author

Choose a reason for hiding this comment

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

Flagging the logic here on how I'm deciding whether or not to display the "success" output. Please let me know if we think the heuristic should be different!

@hughhan1 hughhan1 marked this pull request as ready for review January 13, 2024 07:18
@hughhan1
Copy link
Author

This is my first time attempting to contribute to the Ruff codebase, so please let me know if my logic is misdirected, e.g. at the incorrect abstraction levels.

I'm really trying to familiarize myself with the codebase so any direction or guidance is greatly appreciated!

Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@hughhan1
Copy link
Author

Ahh, I just realized that #8631 already exists, and is probably at the more correct level of abstraction to implement this in!

@zanieb
Copy link
Member

zanieb commented Mar 11, 2024

#8631 does seem more appropriate, we'll try to get that one merged.

Thank you for contributing though! I'm interested in adding those file counts 👀

@zanieb zanieb closed this Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants