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

Extend to multiple searches? #7

Closed
robitalec opened this issue Feb 20, 2020 · 4 comments
Closed

Extend to multiple searches? #7

robitalec opened this issue Feb 20, 2020 · 4 comments

Comments

@robitalec
Copy link

@robitalec robitalec commented Feb 20, 2020

Right now, it seems errorist just searches for the first warning or message.
This came up for me in trying out the package, I was actually more interested in the search results for the second warning..


Reproducible example:

library(errorist)

warn2 <- function() {
  warning('warning one')
  warning('warning two')
}

warn2()
@coatless

This comment has been minimized.

Copy link
Collaborator

@coatless coatless commented Feb 21, 2020

@robitalec yes, only searching the first error and warning for the latest function call is accurate.

Having said this, there are two parts here that are drastically different: error and warning.

Error-wise, it isn't possible to have two errors back-to-back within code execution since the code is exited once a single error is detected. We're able to then use the error handler to trigger an appropriate dispatch.

Warning dispatch is another story. errorist was setup to search the first warning returned as multiple identical warnings may occur in a single function call. Though, this might have been slightly too conservative. Potentially, we could trigger a search on distinct warnings by parsing names(list.warning).

Self-note:

errorist/R/handlers.R

Lines 88 to 96 in 8a41cb8

# Trigger search with added protections since this caller is repeatedly
# called regardless if a new warning is detected.
if (!is.na(warning_contents) &&
(
is.na(.errorist_env$captured_last_search_warning) ||
.errorist_env$captured_last_search_warning != warning_contents
)) {
search_func(warning_contents)
}

@robitalec

This comment has been minimized.

Copy link
Author

@robitalec robitalec commented Feb 21, 2020

Thanks @coatless!
Silly mistake - you never get two errors. Wrote this up a bit quickly clearly, edited above..

Yes - I think we definitely wouldn't want repeated searches from the same warning repeated, but might be useful to search for unique ones, like you say. Thanks!

coatless added a commit that referenced this issue Feb 22, 2020
@coatless coatless closed this in #9 Feb 22, 2020
coatless added a commit that referenced this issue Feb 22, 2020
* Enable searches for multiple warnings. (Close #7)

* Roll new version
@coatless

This comment has been minimized.

Copy link
Collaborator

@coatless coatless commented Feb 22, 2020

@robitalec searching multiple warnings should now work. Please try it out in the development version:

remotes::install_github('r-assist/errorist')
@robitalec

This comment has been minimized.

Copy link
Author

@robitalec robitalec commented Feb 24, 2020

Perfect! That worked @coatless.
Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.