-
Notifications
You must be signed in to change notification settings - Fork 893
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
feat: --quiet option #1945
feat: --quiet option #1945
Conversation
I'll need to properly review to be sure, but first glance suggests it's okay.
This is the entry point for rustup proxies. Proxying is the mechanism by which |
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.
After reviewing, I'm mildly concerned that we have verbose
and quiet
which are not really antonyms of one another. But I'm also not sure of better terminology right now.
I'll await your further updates with tests etc, before I pass judgement further. Renaming options is the easiest of things to do.
Could you also be sure to rebase against new master since I've merged a change which might make the CI more useful.
Ok, thanks for the review =), I'll rebase and update the PR accordingly.
|
7fa3c7e
to
e99b6db
Compare
TestsThe goal of During unit tests stdout and stderr are checked against expected output. However final stdout is the same regardless of whether or not For the lack of a better solution I copied tests from It won't prove the feature works as expected (disables the progress output) but it shows the
NameAs for the option name, wget has a
|
280e638
to
7483f3b
Compare
☔ The latest upstream changes (presumably ec6d1af) made this pull request unmergeable. Please resolve the merge conflicts. |
b0815f7
to
ba40e0b
Compare
ee4bfb0
to
6df8beb
Compare
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.
Just a query about verbose vs quiet then probably mergeable
--quiet disables progress output tests: see `tests/cli-rustup.rs`
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.
LGTM, Thank you 👍
feat: --quiet option
PR Content
issue: #1928
add
--quiet
option. it disables progress console output.TODO
proxy_mode.rs
is the right one.