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

feat: --quiet option #1945

Merged
merged 2 commits into from
Sep 26, 2019
Merged

feat: --quiet option #1945

merged 2 commits into from
Sep 26, 2019

Conversation

PicoJr
Copy link

@PicoJr PicoJr commented Jul 14, 2019

PR Content

issue: #1928

add --quiet option. it disables progress console output.

TODO

  • write unit test for this new option.
  • verify default behavior in proxy_mode.rs is the right one.

@kinnison
Copy link
Contributor

is this a reasonable default?

I'll need to properly review to be sure, but first glance suggests it's okay.

I don't really understand the purpose of proxy_mode.rs

This is the entry point for rustup proxies. Proxying is the mechanism by which rustc cargo etc, are really rustup but we use argv[0] to decide which command to run from inside the detected toolchain. It can result in a download if the detected toolchain isn't installed I think, though in that context, I imagine defaulting to quiet is fine.

@kinnison kinnison self-requested a review July 15, 2019 12:43
Copy link
Contributor

@kinnison kinnison left a 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.

@PicoJr
Copy link
Author

PicoJr commented Jul 28, 2019

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.

  • rebase
  • add unit tests

@PicoJr PicoJr force-pushed the feat-1928 branch 2 times, most recently from 7fa3c7e to e99b6db Compare July 28, 2019 14:57
@PicoJr
Copy link
Author

PicoJr commented Jul 28, 2019

Tests

The goal of --quiet is to suppress progress output.

During unit tests stdout and stderr are checked against expected output.

However final stdout is the same regardless of whether or not --quiet option was used (progress line is flushed out).

For the lack of a better solution I copied tests from cli-rustup.rs: rustup_stable and toolchain_install_is_like_update and used --quiet option.

It won't prove the feature works as expected (disables the progress output) but it shows the --quiet option can be used and produces the same output as before (except for the progress line)

I'm open to suggestions for improving the tests

Name

As for the option name, wget has a --progress=type option.

--quiet could be renamed as --no-progress ?

@bors
Copy link
Contributor

bors commented Sep 11, 2019

☔ The latest upstream changes (presumably ec6d1af) made this pull request unmergeable. Please resolve the merge conflicts.

@PicoJr PicoJr force-pushed the feat-1928 branch 2 times, most recently from b0815f7 to ba40e0b Compare September 12, 2019 11:32
@PicoJr PicoJr force-pushed the feat-1928 branch 2 times, most recently from ee4bfb0 to 6df8beb Compare September 19, 2019 16:57
Copy link
Contributor

@kinnison kinnison left a 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

src/cli/setup_mode.rs Show resolved Hide resolved
src/cli/rustup_mode.rs Show resolved Hide resolved
PicoJr added 2 commits September 25, 2019 20:36
--quiet disables progress output
tests: see `tests/cli-rustup.rs`
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you 👍

@kinnison kinnison merged commit eb694fc into rust-lang:master Sep 26, 2019
@PicoJr PicoJr deleted the feat-1928 branch October 17, 2019 18:29
AJ-Ianozi pushed a commit to AJ-Ianozi/getada-download that referenced this pull request Oct 12, 2023
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.

3 participants