Conversation
10a6f4f to
04a5d55
Compare
DavisVaughan
left a comment
There was a problem hiding this comment.
Tried it out on case_when() and it seems to work pretty nicely!
| pub(crate) fn with_capture<T>(f: impl FnOnce() -> T) -> (T, String) { | ||
| let mut capture = Console::get_mut().start_capture(); | ||
| let result = f(); | ||
| let output = capture.take(); | ||
| drop(capture); | ||
| (result, output) | ||
| } |
There was a problem hiding this comment.
| pub(crate) fn with_capture<T>(f: impl FnOnce() -> T) -> (T, String) { | |
| let mut capture = Console::get_mut().start_capture(); | |
| let result = f(); | |
| let output = capture.take(); | |
| drop(capture); | |
| (result, output) | |
| } | |
| pub(crate) fn with_capture<T>(f: impl FnOnce() -> T) -> (T, String) { | |
| let mut capture = Console::get_mut().start_capture(); | |
| let result = f(); | |
| let output = capture.take(); | |
| (result, output) | |
| } |
That seems a bit superfluous?
| #[cfg(test)] | ||
| mod tests_condition_output { |
There was a problem hiding this comment.
can we move tests to the very bottom? i totally skipped over eval_condition() because i thought this was the end of the file
| // `if` coerces via `asLogicalNoNA` (not the generic `as.logical`) | ||
| // and errors on NA, length != 1, and non-coercible types. | ||
| let code = format!("base::.ark_eval_capture(if ({{ {condition} }}) TRUE else FALSE)"); |
There was a problem hiding this comment.
feels like the right semantics to me
| Ok(val) => (val, conditions, None), | ||
| Err(err) => (true, conditions, Some(format!("Error: {err}"))), |
There was a problem hiding this comment.
I had wondered what this mysterious 3rd diagnostic field was when reading the rest of the code.
...it's just what we get when a conversion from an R bool to Rust bool fails?
That feels like maybe a bit of overkill? Can we just tie that in with conditions or something instead?
I think it would have cleared things up for me quite a bit while reading the rest of the code.

Adds support for conditional breakpoints in the DAP server. When a breakpoint has a condition expression, it is evaluated at hit time in the breakpoint's local environment. The breakpoint only stops execution if the condition is truthy.
The condition is stored as a string in the Rust
Breakpointstruct and evaluated on the R thread via a newps_should_breakregistered function that combines the enabled check with condition evaluation. The DAP mutex is dropped before calling back into R to avoid deadlock.Condition results are coerced via
as.logical()so R's standard truthy/falsy rules apply:0skips,1stops,nrow(df)works naturally. If evaluation errors or the result can't be coerced to scalar logical (e.g. an environment), the breakpoint fires to avoid silently swallowing typos in conditions. From a quick search this seems standard among DAPs (e.g. js/ts, python). If we feel strongly that we should be stricter, we can revert the last commit.Conditions that fail with an error always trigger the breakpoint to avoid silently skipping them. The error is logged in the console.The message is fenced with a breakpoint header that includes a link to the breakpoint location:
If there is any output, warning, or messages, we log in the console too.
QA Notes
Comprehensively tested on the backend side (10 integration tests covering true/false conditions, loop iteration, local variables, error conditions, numeric coercion, mixed breakpoints, and live condition updates).
To test manually:
y <- x * 2with conditionx == 3. Run the loop, should stop only whenxis 3.x > 4without re-sourcing. Run the loop again, should now stop atx == 5.x - 2(should skip whenx == 2since0is falsy).nonexistent— should stop on every hit rather than silently skipping. You should see the error in the Console with a link to jump to the breakpoint.