-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Add a --quiet mode
#19233
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
[ty] Add a --quiet mode
#19233
Conversation
f436dd6 to
daa9309
Compare
| use crate::logging::VerbosityLevel; | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | ||
| pub(crate) struct Printer { |
There was a problem hiding this comment.
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.
crates/ty/src/printer.rs
Outdated
| /// Return the [`Stdout`] stream for general messages. | ||
| pub(crate) fn stdout(self) -> Stdout { |
There was a problem hiding this comment.
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.
| match self.status { | ||
| StreamStatus::Enabled => self.lock = Some(std::io::stdout().lock()), | ||
| StreamStatus::Disabled => self.lock = None, | ||
| } | ||
| self |
There was a problem hiding this comment.
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.
|
|
||
| /// A progress reporter. | ||
| pub trait Reporter: Send + Sync { | ||
| pub trait ProgressReporter: Send + Sync { |
There was a problem hiding this comment.
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.
crates/ty/src/printer.rs
Outdated
| /// Return the [`Stdout`] stream for an important message. | ||
| pub(crate) fn stdout_important(self) -> Stdout { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
crates/ty/src/lib.rs
Outdated
| /// A progress reporter for `ty check`. | ||
| #[derive(Default)] | ||
| struct IndicatifReporter(Option<indicatif::ProgressBar>); | ||
| struct IndicatifReporter { |
There was a problem hiding this comment.
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.
|
b70c469 to
1d8b17c
Compare
|
I plan on reviewing this tomorrow morning |
There was a problem hiding this 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
--quietbecause 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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).
crates/ty/src/printer.rs
Outdated
| /// Return the [`Stdout`] stream for an important message. | ||
| pub(crate) fn stdout_important(self) -> Stdout { |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
MichaReiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
…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)
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.
Adds a
--quietflag 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
Printerabstraction used in uv, which is intended to replace all direct usage ofstd::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