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

Add option for suppressing styler communication #640

Merged
merged 11 commits into from May 13, 2020

Conversation

michaelquinn32
Copy link
Contributor

This implements FR #637

@michaelquinn32
Copy link
Contributor Author

Hi Lorenz!

Thanks for the patience. I finally got around to this. One thing. I looked a little bit, but couldn't find if or where styler's communication was tested. Mind sharing a code pointer?

Thanks!

@lorenzwalthert
Copy link
Collaborator

Thanks. Two things to debate:

  • You implemented as a boolean option. This only allows two modes. Do we ever want to support more than two modes? I.e. moderately verbose?
  • In {usethis}, the option is called usethis.quiet. I don't know if the tidyverse has embraced standardisation for options that manage communication. Maybe we should investigate that.

Tests for communication are here: https://github.com/r-lib/styler/blob/master/tests/testthat/test-public_api.R

@michaelquinn32
Copy link
Contributor Author

I can't think of a situation where "less verbose" makes sense. For my use case, either the messaging is useful (which it almost always is), or it is interfering with something like stdout.

I'm happy to update the flag name. Alignment with other packages is good.

NEWS.md Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented May 8, 2020

Codecov Report

Merging #640 into master will decrease coverage by 0.06%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #640      +/-   ##
==========================================
- Coverage   90.41%   90.35%   -0.07%     
==========================================
  Files          47       47              
  Lines        2181     2188       +7     
==========================================
+ Hits         1972     1977       +5     
- Misses        209      211       +2     
Impacted Files Coverage Δ
R/addins.R 0.00% <0.00%> (ø)
R/zzz.R 0.00% <0.00%> (ø)
R/communicate.R 92.30% <100.00%> (+2.30%) ⬆️
R/transform-files.R 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85a81fb...3400e9a. Read the comment docs.

@lorenzwalthert
Copy link
Collaborator

How come that a significant part of NEWS.md got reformatted in 32f659c?

@michaelquinn32
Copy link
Contributor Author

Because I have an autoformatter that needs to be manually disabled 🤦‍♂️.

Sorry, it's fixed.

@lorenzwalthert
Copy link
Collaborator

Out of curiosity: Which formatter do you use? Could we add that to {precommit} as a hook?

@lorenzwalthert lorenzwalthert merged commit 8dad103 into r-lib:master May 13, 2020
@lorenzwalthert
Copy link
Collaborator

Thanks @michaelquinn32.

@michaelquinn32
Copy link
Contributor Author

A precommit would be great!

I'm using an internal formatter, but I don't think it matters all that much. prettier, for example, supports markdown: https://github.com/prettier/prettier

@lorenzwalthert
Copy link
Collaborator

They already made a pre-commit compatible hooks themself: https://prettier.io/docs/en/precommit.html#option-3-pre-commithttpsgithubcompre-commitpre-commit

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.

None yet

3 participants