Skip to content

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Jul 9, 2025

Adds a --quiet flag which silences diagnostic, warning logs, and messages like "all checks passed" while retaining summary messages that indicate problems, e.g., the number of diagnostics.

I'm a bit on the fence regarding filtering out warning logs, because it can omit important details, e.g., the message that a fatal diagnostic was encountered. Let's discuss that in #19233 (comment)

The implementation recycles the Printer abstraction used in uv, which is intended to replace all direct usage of std::io::stdout. See #19233 (comment)

I ended up futzing with the progress bar more than I probably should have to ensure it was also using the printer, but it doesn't seem like a big deal. See #19233 (comment)

Closes astral-sh/ty#772

@zanieb zanieb added the ty Multi-file analysis & type inference label Jul 9, 2025
@zanieb zanieb changed the title Add a --quiet mode [ty] Add a --quiet mode Jul 9, 2025
@zanieb zanieb force-pushed the zb/printer branch 4 times, most recently from f436dd6 to daa9309 Compare July 9, 2025 14:09
use crate::logging::VerbosityLevel;

#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
pub(crate) struct Printer {
Copy link
Member Author

@zanieb zanieb Jul 9, 2025

Choose a reason for hiding this comment

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

The Printer abstraction is lifted from uv, but is a little different here because we reuse the existing VerbosityLevel enum, support locking the stream, and don't need stderr support yet.

Comment on lines 58 to 59
/// Return the [`Stdout`] stream for general messages.
pub(crate) fn stdout(self) -> Stdout {
Copy link
Member Author

Choose a reason for hiding this comment

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

Initially I wrote a stderr handler too, but we don't actually need it here yet.

Comment on lines 84 to 88
match self.status {
StreamStatus::Enabled => self.lock = Some(std::io::stdout().lock()),
StreamStatus::Disabled => self.lock = None,
}
self
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't support locking in uv's printer, but in ty we lock stdout for the hot loop of printing diagnostics which seems reasonable and worth retaining.

@@ -113,7 +113,7 @@ pub struct Project {
}

/// A progress reporter.
pub trait Reporter: Send + Sync {
pub trait ProgressReporter: Send + Sync {
Copy link
Member Author

Choose a reason for hiding this comment

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

I just renamed this for clarity, as I initially thought this was used to report more than progress.

Comment on lines 47 to 48
/// Return the [`Stdout`] stream for an important message.
pub(crate) fn stdout_important(self) -> Stdout {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't love this name, but it's a simple way to handle things for now. I could see us doing things like stream_for_summary, stream_for_diagnostics, etc. in the future?

Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined to go with stream_for_summary as those are the main messages that should still be written when -q is used

Copy link
Member Author

Choose a reason for hiding this comment

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

Works for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I didn't do this initially is because stream_for_summary doesn't really fit the use case for something like the version command

Copy link
Member Author

Choose a reason for hiding this comment

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

(I'll come up with some names though)

@@ -383,27 +385,54 @@ impl MainLoop {
}

/// A progress reporter for `ty check`.
#[derive(Default)]
struct IndicatifReporter(Option<indicatif::ProgressBar>);
struct IndicatifReporter {
Copy link
Member Author

@zanieb zanieb Jul 9, 2025

Choose a reason for hiding this comment

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

I got a bit in the weeds here handling the progress bar. We have handling for it in uv, and I thought it'd be reasonable to copy over. Arguably, we could just cut scope here and continue to use the DummyReporter, but I think it's nice to consolidate the logic for showing and hiding output in the main binary.

Copy link
Contributor

github-actions bot commented Jul 9, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@zanieb zanieb force-pushed the zb/printer branch 3 times, most recently from b70c469 to 1d8b17c Compare July 9, 2025 15:48
@zanieb zanieb marked this pull request as ready for review July 9, 2025 16:45
@AlexWaygood AlexWaygood removed their request for review July 9, 2025 16:57
@MichaReiser
Copy link
Member

I plan on reviewing this tomorrow morning

@MichaReiser MichaReiser added the cli Related to the command-line interface label Jul 10, 2025
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks. There are three changes we should make before landing but this otherwise looks good to me.

  • We should avoid rendering the diagnostics when running in --quiet because that's a lot of unnecessary work otherwise
  • Disallow -qq (and variance) until we actually do support other quiet modes
  • I think there's one summary message that now gets omitted that should be rendered as part of --quiet

global = true,
overrides_with = "verbose",
)]
quiet: u8,
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to keep this as a bool flag. It seems more confusing to users if they can use -qq but it doesn't change the behavior in anyway

Copy link
Member Author

@zanieb zanieb Jul 10, 2025

Choose a reason for hiding this comment

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

I'd like to push back on that, I think we'll want a --qq silent mode to match uv and this is forwards compatible. You can also provide -vvvvvvv and it doesn't do anything more.

Copy link
Member

@MichaReiser MichaReiser Jul 10, 2025

Choose a reason for hiding this comment

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

I think we'll want a --qq silent mode

What does this do? Does it silence everything?

I don't feel too strongly. I just think both approaches are forward compatible and I'm not entirely sure yet if we indeed want -qq in the future (because the use case isn't clear to me not because I'm oposed to it)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, "silent" mode.

It saves you from piping the command's output to /dev/null which is error prone in various shells (as we've discovered in our install scripts).

Comment on lines 47 to 48
/// Return the [`Stdout`] stream for an important message.
pub(crate) fn stdout_important(self) -> Stdout {
Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined to go with stream_for_summary as those are the main messages that should still be written when -q is used

write!(stdout, "{}", diagnostic.display(db, &display_config))?;
// Only render diagnostics if they're going to be displayed, since doing
// so is expensive.
if stdout.is_enabled() {
Copy link
Member

@MichaReiser MichaReiser Jul 10, 2025

Choose a reason for hiding this comment

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

We should move this check outside the for loop as the result is always the same for all diagnostics.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, never mind. It's in here because of the max_severity check below

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep haha, that broke the tests and I was like.. oh mhm

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thank you

@zanieb zanieb merged commit 965f415 into main Jul 10, 2025
37 checks passed
@zanieb zanieb deleted the zb/printer branch July 10, 2025 14:40
UnboundVariable pushed a commit to UnboundVariable/ruff that referenced this pull request Jul 11, 2025
…re_help

* 'main' of https://github.com/astral-sh/ruff:
  Add simple integration tests for all output formats (astral-sh#19265)
  [`flake8-return`] Fix false-positive for variables used inside nested functions in `RET504`  (astral-sh#18433)
  [ty] Add a `--quiet` mode (astral-sh#19233)
  Treat form feed as valid whitespace before a line continuation (astral-sh#19220)
  [ty] Make `check_file` a salsa query (astral-sh#19255)
  [ty] Consolidate submodule resolving code between `types.rs` and `ide_support.rs` (astral-sh#19256)
  [ty] Remove countme from salsa-structs (astral-sh#19257)
  [ty] Improve and document equivalence for module-literal types (astral-sh#19243)
  [ty] Optimize protocol subtyping by removing expensive and unnecessary equivalence check from the top of `Type::has_relation_to()` (astral-sh#19230)
  [ty] Ecosystem analyzer: parallelize, fix race condition (astral-sh#19252)
  [ty] Add completion kind to playground (astral-sh#19251)
  [ty] Deploy ecosystem diff to Cloudflare pages (astral-sh#19234)
  [ty] Add semantic token provider to playground (astral-sh#19232)
zanieb added a commit that referenced this pull request Jul 15, 2025
This matches uv's behavior.

Briefly discussed at
#19233 (comment)

I think the most useful case is to avoid piping to `/dev/null` which
hard to do properly in a cross-platform script.
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 ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --quiet CLI flag to ty check
2 participants