Conversation
lionel-
left a comment
lionel-
left a comment
There was a problem hiding this comment.
I have a few thoughts about the new handling for character vectors. Although it is defensive regarding the presence of empty names, I'm a bit concerned that simple changes to the R representation (a name slips in) results in a completely different json representation. Also since the mapping from named vector to json map is lossy when there are duplicate names, this could lead to surprising behaviour or bugs in extension code.
But more generally, since jsonlite is the most popular serialisation package, I think it would be good to follow what it is doing, to follow the principle of least surprise.
- Lists: Array/Object depending on presence of names (as we currently do). This suffers from type unstability for pragmatic reasons.
- Vectors: Always as arrays, names are ignored. To get an Object, use
as.list().
crates/harp/src/json.rs
Outdated
|
|
||
| #[test] | ||
| #[allow(non_snake_case)] | ||
| fn test_json_named_character_vectors() { |
There was a problem hiding this comment.
Missing tests:
- Empty character vector (empty array)
- Character vector of size one (still an array, not a scalar)
- Character vector with
NAnames (which core R methods sometimes introduce) - Character vector with duplicate names, should test that last one wins.
crates/ark/src/ui/ui.rs
Outdated
| harp::Error::TryCatchError { message, .. } => message.clone(), | ||
| harp::Error::ParseError { message, .. } => message.clone(), | ||
| harp::Error::ParseSyntaxError { message } => message.clone(), |
There was a problem hiding this comment.
I think the clones can be avoided if you match on err instead of &err
crates/harp/src/json.rs
Outdated
| let names = obj.names()?; | ||
| let all_empty = names.iter().all(|name| match name { | ||
| Some(name) => name.is_empty(), | ||
| None => true, |
There was a problem hiding this comment.
NA names become None here IIUC? This would be the expected behaviour, just checking (and should also be tested).
crates/amalthea/src/comm/ui_comm.rs
Outdated
| /// The result of evaluating the code | ||
| EvaluateCodeReply(serde_json::Value), |
There was a problem hiding this comment.
By the way there's one command that I was using all the time in Emacs and that I'd like to implement in Positron. It's a version of "evaluate statement" that inserts the output of the evaluation below the statement, with #> prefix.
IIUC it's not currently possible to implement this from an extension because the evaluation needs to happen in the background (silently) but the output should still be captured and returned to the caller. It looks like that use case is not covered by posit-dev/positron#7112.
To support this case, perhaps we could consider returning a structured value with result and output fields. The output field could be opt-in by language in case the kernel can't easily support it. See #1064 for an example of how to evaluate with output and condition capture.
In any case, wrapping the return value in a struct would be more future-proof, allowing new fields to be added as needed.
|
@lionel- I implemented an output capture system, and also rolled back the changes to the JSON serializer. Thanks for reviewing! |
crates/ark/src/ui/ui.rs
Outdated
| let result = r_task(|| { | ||
| let evaluated = parse_eval_global(¶ms.code)?; | ||
| Value::try_from(evaluated) | ||
| // Set up a raw connection to capture printed output via sink(). |
There was a problem hiding this comment.
Can we use Console::start_capture() instead?
Warnings are a complication to be aware of, as by defaults they are emitted when R goes back to idle.
I realised in https://github.com/posit-dev/ark/pull/1064/changes#r2896192009 that the warning capture I've implemented for conditional breakpoints can be simplified by having the output capture guard set options(warn = 1) while output is being captured. This causes warnings to be emitted immediately instead of being buffered by R. So if you switch to the console capture mechanism, it should take care of warnings once that change is merged.
Warnings will behave more like messages and regular output in that they will be interleaved in the captured output, but I think that's acceptable.
There was a problem hiding this comment.
Impl in 9ccdd0d, let me know what you think. The console isn't available in unit tests, so I added some tests closer to the integration layer (i.e. using a "UI Comm").
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
LGTM, I'll let @lionel- have the final say since he had more comments about it!
| let output = capture.take(); | ||
| drop(capture); |
There was a problem hiding this comment.
@lionel- it might be nice to have capture.consume() which returns a String like take() but also consumes self and drops it - like a one shot capture so you don't have to think about dropping as much.
There was a problem hiding this comment.
hmm, or capture.into_string()?
The explicit drop is a clearer signal of the side effect though.
| let output = capture.take(); | ||
| drop(capture); |
There was a problem hiding this comment.
hmm, or capture.into_string()?
The explicit drop is a clearer signal of the side effect though.
|
If you'd like you can rebase on main to make a test with warning capture. The Console output capture mechanism now automatically sets the option to emit warnings immediately instead of letting R buffer them. |
3 participants
This change adds an API for evaluating code to Ark.
Most of the change is actually just getting smarter about type coercion, so that named character vectors get serialized into JSON objects instead of losing their names.
Part of posit-dev/positron#7112.