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! |
2 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.