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

Put user-facing output under control of 'usethis.quiet' option #424

Merged
merged 9 commits into from Aug 7, 2018

Conversation

@jennybc
Copy link
Member

@jennybc jennybc commented Aug 3, 2018

Fixes #416 Need way to turn off messaging globally for usethis functions

Well, this was pretty satisfying!

I'm asking for review just to make sure I implemented as discussed and to see if there's anything else I can do along these lines to make life easier for someone using usethis in another package.

I'm also continuing to record implicit design principles explicitly in principles.md.

All of usethis's user-facing output goes through the cat_line() helper. I've put it under the control of a undocumented option "usethis.quiet".

So to use usethis functions quietly in a function do:

withr::local_options(list(usethis.quiet = TRUE))

cc @jimhester @isteves

@jennybc jennybc requested review from hadley and jimhester Aug 3, 2018
jennybc added 3 commits Aug 4, 2018
@jennybc jennybc force-pushed the usethis-quiet branch from 85b1eb4 to 0e52554 Aug 4, 2018
You might also notice that usethis communicates with the user via `cat()` instead of `message()`. Why?

* Pragmatic explanation: default styling of `message()` (at least in RStudio) is red, which suggests that something is wrong. We prefer default styling to be more neutral and less alarmist.
* Principled explanation: if one diverts where various streams go, `cat()` follows printed output, whereas `message()` goes to standard error.

This comment has been minimized.

@jimhester

jimhester Aug 6, 2018
Member

FWIW I don't agree with this principal, I think these status messages should be using message() (and stderr) and assuming RStudio / the Windows console did not re-color them I would push harder to make them use message().

If usethis was a normal unix program I would expect these user facing messages to be on stderr as they would be with message().

This comment has been minimized.

@jennybc

jennybc Aug 6, 2018
Author Member

Yeah, you are not wrong. I just wrote this down because Irene had asked (she's using usethis in the package for Tidies of March) and I decide to record the rationale, such as it is. This would not be hard to change, since everything is so centralized.

R/style.R Outdated
cat_line <- function(...) {
quiet <- getOption("usethis.quiet", default = FALSE)

This comment has been minimized.

@jimhester

jimhester Aug 6, 2018
Member

Wonder if it would be worth promoting this to an argument (with the same default value), so you could call cat_line(quiet = FALSE). e.g.

cat_line <- function(..., quiet = getOption("usethis.quiet", default = FALSE)) {

Maybe only useful for internal testing in usethis, but I think it is a little more natural.

This comment has been minimized.

@jennybc

jennybc Aug 6, 2018
Author Member

For this to be productive internally, I'd need to add a quiet argument to todo() and done() and then this could happen in a few places:

Before:

if (!quiet) {
    done("Adding {collapse(value(new))} to {value(proj_rel_path(path))}")
}

After:

done("Adding {collapse(value(new))} to {value(proj_rel_path(path))}", quiet = quiet)

To be useful externally, I'd need to add quiet to user-facing use_*() functions.

But I see how that is a nicer definition of cat_line() itself, even if I don't do the other bits.

This comment has been minimized.

@jennybc

jennybc Aug 6, 2018
Author Member

And ... done.

This comment has been minimized.

@jennybc

jennybc Aug 6, 2018
Author Member

OK I did that, but one downside is that a future usethis developer could make a direct call to cat_line() and use quiet = FALSE argument and the "usethis.quiet = TRUE" option won't have the promised effect of silence. But presumably they'd either suffer immediately in tests or a downstream maintainer would raise a concern.

@jennybc jennybc merged commit b0df5f1 into master Aug 7, 2018
6 checks passed
6 checks passed
codecov/patch 100% of diff hit (target 61.95%)
Details
codecov/project 61.98% (+0.02%) compared to 00ff3c4
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jennybc jennybc deleted the usethis-quiet branch Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants