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

cli updates for bootci.R #516

Merged
merged 14 commits into from
Sep 11, 2024
Merged

cli updates for bootci.R #516

merged 14 commits into from
Sep 11, 2024

Conversation

Dpananos
Copy link
Contributor

@Dpananos Dpananos commented Aug 15, 2024

closes #504

I've tried to use pluralization where possible.

Note that running testthat::test_file('tests/testthat/test-bootci.R') results in one failure

[ FAIL 1 | WARN 0 | SKIP 0 | PASS 52 ]

── Failure (test-bootci.R:138:3): Upper & lower confidence interval does not contain NA ──
Snapshot of code has changed:
old[4:7] vs new[4:7]
    Warning:
    Recommend at least 1000 non-missing bootstrap resamples for term `mean`.
    Error in `pctl_single()`:
-   ! All statistics have missing values..
+   ! All statistics have missing values.

* Run testthat::snapshot_accept('bootci') to accept the change.
* Run testthat::snapshot_review('bootci') to interactively review the change.
[ FAIL 1 | WARN 0 | SKIP 0 | PASS 52 ]

one of the tests expects two periods. I wasn't sure if that was a typo or not. I can switch back to using 2 periods or I can edit the test. Let me know!

@Dpananos
Copy link
Contributor Author

I added back the extra period to get the tests to pass.

@Dpananos
Copy link
Contributor Author

Need to format with {.arg} as mentioned here #518 (comment)

@hfrick
Copy link
Member

hfrick commented Aug 15, 2024

I'm looking at this PR now, but we definitely only need one period at the end! 😆

@Dpananos
Copy link
Contributor Author

Dpananos commented Aug 15, 2024

@hfrick I think this is ready for review now. I've updated the arguments to use the {.arg} formatting, and I've caught some more rlang::aborts I initially missed.

I also remove the extra period.

Also, I’m about to board a flight so if I don’t respond then that’s why. Someone else can feel free to pick this up if I don’t respond in time!

Copy link
Member

@hfrick hfrick left a comment

Choose a reason for hiding this comment

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

Thanks for putting in the styling changes already! Could you also take a look at the one code comment here and not namespace cli::cli_abort() here (because we already import it)?

R/bootci.R Outdated Show resolved Hide resolved
@Dpananos
Copy link
Contributor Author

Dpananos commented Aug 16, 2024

Checks still might fail due to the multiple periods. I've rolled back changes I made to the snapshot since those were in error.

See failing tests below

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-bootci.R:138:3'): Upper & lower confidence interval does not contain NA ──
Snapshot of code has changed:
old[4:7] vs new[4:7]
    Warning:
    Recommend at least 1000 non-missing bootstrap resamples for term `mean`.
    Error in `pctl_single()`:
-   ! All statistics have missing values..
+   ! All statistics have missing values.

Also I switched some of the phrasing in the check_num_resamples warning so I could use pluralization. The checks are failing because of that

── Failure (test-bootci.R:143:3): Upper & lower confidence interval does not contain NA ──
Snapshot of code has changed:
old[2:7] vs new[2:7]
    int_t(bt_resamples, res)
  Condition
    Warning:
-   Recommend at least 500 non-missing bootstrap resamples for term `mean`.
+   Recommend at least 500 non-missing bootstrap resamples for `mean` term.
    Error in `t_single()`:
    ! All statistics have missing values.

@hfrick
Copy link
Member

hfrick commented Sep 11, 2024

Thanks @Dpananos for the PR! I've fiddled a little with the pluralization for the term{?s} bit and now we're set!

@hfrick hfrick changed the title Cli bootci cli updates for bootci.R Sep 11, 2024
@hfrick hfrick merged commit 5c84b97 into tidymodels:main Sep 11, 2024
12 checks passed
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 27, 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.

Use cli errors for R/bootci.R
2 participants