-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[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.
crates/ty/src/printer.rs
Outdated
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.
@@ -113,7 +113,7 @@ pub struct Project { | |||
} | |||
|
|||
/// 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
@@ -383,27 +385,54 @@ impl MainLoop { | |||
} | |||
|
|||
/// 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
--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, |
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
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
--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 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