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

Replace base R message/warning/stop with {cli} #762

Merged
merged 16 commits into from
Sep 1, 2024
Merged

Replace base R message/warning/stop with {cli} #762

merged 16 commits into from
Sep 1, 2024

Conversation

jamesmbaazam
Copy link
Contributor

Description

This PR closes #546.

This PR replaces the use of base condition signalling with those provided by {cli} as it offers more powerful features such as the ability to display a warning every few hours.

As part of this PR, I've updated the .lintr config file to catch future uses of base messaging. This was really useful in the refactoring exercise as it caught all occurrences. There were some false positives in the regional_epinow.R, so I embraced them in #nolint undesirable_function_linter tags to counter the configs I've set up.

Initial submission checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have tested my changes locally (using devtools::test() and devtools::check()).
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required and rebuilt docs if yes (using devtools::document()).
  • I have followed the established coding standards (and checked using lintr::lint_package()).
  • I have added a news item linked to this PR.

After the initial Pull Request

  • I have reviewed Checks for this PR and addressed any issues as far as I am able.

@jamesmbaazam jamesmbaazam added the enhancement New feature or request label Aug 29, 2024
.lintr Show resolved Hide resolved
DESCRIPTION Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

This looks great. Like the lintr as the package check and the use of colour highlighting. You have some very unhappy CI which I expect is due to error and warning testing needing to be updated.

@jamesmbaazam
Copy link
Contributor Author

Sorry, I should have marked this as a draft. Have a few simple fixes then we can try the new changes and see if happy before merging, or we can merge this and apply fancier stuff in another run. {cli} is addictive and I'm trying not to go crazy.

@jamesmbaazam jamesmbaazam marked this pull request as draft August 30, 2024 10:43
@jamesmbaazam jamesmbaazam changed the title Replace base messaging with {cli} Replace base R message/warning/stop with {cli} Aug 30, 2024
Co-authored-by: Sam Abbott <s.e.abbott12@gmail.com>
@seabbs
Copy link
Contributor

seabbs commented Aug 30, 2024

Have a few simple fixes then we can try the new changes and see if happy before merging

Ha I know I think this and do fancier stuff in a new PR

@jamesmbaazam jamesmbaazam marked this pull request as ready for review September 1, 2024 10:32
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Nice. Looks good.

@seabbs seabbs added this pull request to the merge queue Sep 1, 2024
Merged via the queue into main with commit e868977 Sep 1, 2024
9 checks passed
@seabbs seabbs deleted the cli branch September 1, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to {cli} for messaging
2 participants