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

better call routing for errors #1214

Merged
merged 19 commits into from
Oct 22, 2024
Merged

better call routing for errors #1214

merged 19 commits into from
Oct 22, 2024

Conversation

topepo
Copy link
Member

@topepo topepo commented Oct 21, 2024

Focuses on functions called by user-facing functions.

A few more message conversions enabled us to remove glue from depends.

Also removed an unused function for bartMachine (which we don't support)

@topepo topepo marked this pull request as ready for review October 21, 2024 19:52
topepo pushed a commit to tidymodels/extratests that referenced this pull request Oct 21, 2024
@topepo
Copy link
Member Author

topepo commented Oct 21, 2024

After this one, I'll have a corresponding (but trivial) extratests PR ☝️ .

Copy link
Contributor

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

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

HECK YEAH

"Object of class {.cls class(x)[1]} cannot be coerced to
object of class {.cls class(ref)[1]}.",
"Object of class {.cls {class(x)[1]}} cannot be coerced to
object of class {.cls {class(ref)[1]}}.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

Even better, I changed it to {.obj_type_friendly {x}}

R/condense_control.R Outdated Show resolved Hide resolved
R/contr_one_hot.R Outdated Show resolved Hide resolved
R/contr_one_hot.R Outdated Show resolved Hide resolved
@@ -258,7 +258,7 @@ make_form_call <- function(object, env = NULL) {
}

# TODO we need something to indicate that case weights are being used.
make_xy_call <- function(object, target, env) {
make_xy_call <- function(object, target, env, call = rlang::caller_env()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll just make this comment once to apply throughout the PR, but rlang is imported with @import rlang so we don't need to namespace rlang for these!

Copy link
Member Author

Choose a reason for hiding this comment

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

It all comes back to psock... we started namespacing everything in these packages because importing was not enough.

tests/testthat/test-condense_control.R Outdated Show resolved Hide resolved
tests/testthat/test-contr_one_hot.R Outdated Show resolved Hide resolved
tests/testthat/test-contr_one_hot.R Outdated Show resolved Hide resolved
tests/testthat/test-misc.R Show resolved Hide resolved
R/glmnet-engines.R Outdated Show resolved Hide resolved
@topepo topepo merged commit 58e4329 into main Oct 22, 2024
10 checks passed
@topepo topepo deleted the pass-call-through branch October 22, 2024 12:01
topepo added a commit to tidymodels/extratests that referenced this pull request Oct 22, 2024
Co-authored-by: ‘topepo’ <‘[email protected]’>
Copy link

github-actions bot commented Nov 6, 2024

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 Nov 6, 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.

2 participants