-
Notifications
You must be signed in to change notification settings - Fork 187
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
Use {cli}
progress bar
#2641
Use {cli}
progress bar
#2641
Conversation
Re testing: Is there a way to hook into progress bar updates, or supply a "testing" progress bar to cli? Otherwise this seems hard to properly and robustly test - especially the animation. |
I don't see a need to test thing (formally/in testthat suite), besides ensuring it doesn't cause any error. We can rely on user reports of things "looking bad" IMO. |
Technically, this is ready for a review. The only thing I am unsure of is whether we should be using an experimental feature from |
|
I had already confirmed this with Gabor: |
Thanks, I missed the linked issue! |
R/lint.R
Outdated
} else { | ||
lints <- lapply( | ||
files, | ||
function(file) { |
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.
Should I just nolint this?
It's catch-22 situation:
- if I use anonymous function, it triggers
unnecessary_lambda_linter()
lint - if I don't, it leads to
Error: The use of linters of class 'function' was deprecated in lintr version 3.0.0
error
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.
that's interesting! I don't understand why the error. Yes, mark for # nolint
, but please tag it with a TODO+follow-up bug to dive deeper on that to see if there's some bug.
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.
I decided to nolint it.
R/lint.R
Outdated
} else { | ||
lints <- lapply( | ||
files, | ||
function(file) { |
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.
that's interesting! I don't understand why the error. Yes, mark for # nolint
, but please tag it with a TODO+follow-up bug to dive deeper on that to see if there's some bug.
Oops, just noticed my comment wound up in pending state and only got sent now! Sorry... |
TODO:
cli_progress_along()
still experimental? cli#709)figure out how to test this feature