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 columns fmt_fn, warning, and errors to ard_attributes output #343

Merged

Conversation

jtalboys
Copy link
Collaborator

@jtalboys jtalboys commented Sep 30, 2024

What changes are proposed in this pull request?

Provide more detail here as needed.

closes #327

Please let me know if there is anything else to change here. I'm not 100% sure I'm setting the fmt_fn column right? Also had a look at the docs for ard_attributes() to see if any changes were required but they only talk about the added attributes, don't mention the columns of the output.


Pre-review Checklist (if item does not apply, mark is as complete)

  • All GitHub Action workflows pass with a ✅
  • PR branch has pulled the most recent updates from master branch: usethis::pr_merge_main()
  • If a bug was fixed, a unit test was added.
  • Code coverage is suitable for any new functions/features (generally, 100% coverage for new code): devtools::test_coverage()
  • Request a reviewer

Reviewer Checklist (if item does not apply, mark is as complete)

  • If a bug was fixed, a unit test was added.
  • Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
  • Code coverage is suitable for any new functions/features: devtools::test_coverage()

When the branch is ready to be merged:

  • Update NEWS.md with the changes from this pull request under the heading "# cards (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (see NEWS.md for examples).
  • All GitHub Action workflows pass with a ✅
  • Approve Pull Request
  • Merge the PR. Please use "Squash and merge" or "Rebase and merge".

Copy link
Collaborator

@ddsjoberg ddsjoberg left a comment

Choose a reason for hiding this comment

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

Thank you so much @jtalboys !! These look great! I just have one comment and the request below.

While reviewing I noticed that I had never added a check that the user-passed label argument is a named list. Can you also add a check like this?

  if (!is_empty(label)) {
    if (!is.list(label) || !is_named(label) || some(label, \(x) !is_string(x))) {
      cli::cli_abort(
        "The {.arg label} argument must be a named list with each element a string.",
        call = get_cli_abort_call()
      )
    }

R/ard_attributes.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@ddsjoberg ddsjoberg left a comment

Choose a reason for hiding this comment

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

This is wonderful, thank you @jtalboys !

@ddsjoberg ddsjoberg merged commit 6752272 into insightsengineering:main Oct 2, 2024
34 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add warning and error columns to ard_attributes() output
2 participants