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

Make verbose controllable by an option #94

Closed
maelle opened this issue Jan 8, 2021 · 8 comments · Fixed by #92
Closed

Make verbose controllable by an option #94

maelle opened this issue Jan 8, 2021 · 8 comments · Fixed by #92

Comments

@maelle
Copy link
Contributor

maelle commented Jan 8, 2021

cc @Athanasiamo -- does it sound like a good idea?

@drmowinckels
Copy link
Member

yes! Im my growing PR for pro #92 there is a commit for it

@maelle
Copy link
Contributor Author

maelle commented Jan 8, 2021

in your PR it is an argument, not an option yet or am I missing something?

I mean that the default value of the argument would be something with getOption("meetupr.verbose")

@maelle maelle mentioned this issue Jan 8, 2021
@drmowinckels
Copy link
Member

ah right, sorry I misunderstood. I'm not so used to working with options rather than arguments, so I'm not sure what would be the best, tbh. I'll let you decide.

@maelle
Copy link
Contributor Author

maelle commented Jan 8, 2021

or, we could make FALSE the default, or make it TRUE by default when the session is interactive. 🤔

@drmowinckels
Copy link
Member

what I like with arguments is that is more straight forward for the user how to silence it if they want, options can be a little less accessible.

@maelle
Copy link
Contributor Author

maelle commented Jan 8, 2021

I'd still have both, since the argument would read the option, you could still use the argument.

@drmowinckels
Copy link
Member

ok, that sounds like a nice idea

@maelle
Copy link
Contributor Author

maelle commented Jan 8, 2021

I'll wait until your PR is merged since it introduces the verbose argument.

I think its default value should be verbose = getOption("meetupr.verbose", rlang::is_interactive())

  • this way one can still use the argument
  • one can toggle the value via a project/user level Rprofile
  • the default is still TRUE in interactive session.
  • rlang::is_interactive() can be mocked in tests so easier to test we supress messages when we should.

@maelle maelle pinned this issue Jan 8, 2021
@maelle maelle unpinned this issue Jan 12, 2021
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 a pull request may close this issue.

2 participants