Extending the variables pane - Inspect #561
Conversation
| // something that is not a call or a symbol because it would | ||
| // have been handled in Binding::new() |
There was a problem hiding this comment.
Do you have an example of what one of these non call/symbol promise things is? Is it something we need to account for in Binding::new()?
There was a problem hiding this comment.
I didn't write this code, unfortunatelly this appears as lines changed due to moving for refactoring.
Also can't think of an example :(
There was a problem hiding this comment.
I guess parser literals could be stored in prexpr, e.g. in identity(1), in non-compiled code? (The compiler unwraps promises to constants)
| let child: RObject = | ||
| harp::try_catch(|| unsafe { R_do_slot(object.sexp, name) }.into())?; | ||
| return Ok(EnvironmentVariableNode::Concrete { object: child }); |
There was a problem hiding this comment.
There's also R_has_slot() if you are worried about the slot not existing
There was a problem hiding this comment.
I also didn't change that. I'm think the variables.rs file definitely deserves this kind of look, but we might want to do it in a separate PR, so we can track possible problems easier.
dfalbel
force-pushed
the
feature/extend-variables
branch
from
96e9917 to
e6e2e18
Compare
dfalbel
force-pushed
the
feature/extend-variables-inspect2
branch
from
6938088 to
b3a429f
Compare
|
I lacked git skills here , so I had to squash the changes in this PR. The history became completely meaningless due to conflicts when rebasing. |
| // something that is not a call or a symbol because it would | ||
| // have been handled in Binding::new() |
There was a problem hiding this comment.
I guess parser literals could be stored in prexpr, e.g. in identity(1), in non-compiled code? (The compiler unwraps promises to constants)
crates/ark/src/variables/variable.rs
Outdated
| // When building the children list of nodes that use a custom `get_children` method, the access_key is | ||
| // formatted as "custom-{index}-{length(name)}-{name}". If the access_key has this format, we call the custom `get_child_at`, | ||
| // method, if there's one available: | ||
| let result = local!({ |
There was a problem hiding this comment.
We avoid local! since blocks in macros cause difficulties with formatting / assistance.
There was a problem hiding this comment.
Also I don't see a ? so local is not needed?
crates/ark/src/variables/variable.rs
Outdated
| let parsed_access_key: Vec<&str> = access_key.splitn(4, '-').collect(); | ||
|
|
||
| if parsed_access_key.len() != 4 { | ||
| return Ok(None); |
There was a problem hiding this comment.
Should that be logged? Same question for other failure modes.
There was a problem hiding this comment.
Or I guess: Should that be an Err()?
There was a problem hiding this comment.
In this first specific case no, because that just means that the access key does not come from a 'custom' inspect method.
The others could be logged, but in theory they could still be false positives (even though unlikely), an object could have an access key that's custom-key-a-b and get parsed until the below, so my hunch is that we don't need to do anything here.
| // TODO: use ? here, but this does not return a crate::error::Error, so | ||
| // maybe use anyhow here instead ? | ||
| let row_index = path_element.parse::<isize>().unwrap(); | ||
| // R6 objects may be accessed with special elements called <methods> and <private>. |
There was a problem hiding this comment.
Do you think we could move all that code to the new Ark methods?
It'd be nice to move all that complexity to the R side.
There was a problem hiding this comment.
Yes, I have an initial implementation here: https://github.com/dfalbel/testVariablesPaneExtension/blob/main/R/R6.R
It's still pretty much a draft and depends a lot on rlang, but we could move it to ark and inron it out. I'd rather do it in a follow up PR though.
| match name.as_str() { | ||
| "<private>" => { | ||
| let env = Environment::new(object); | ||
| let enclos = Environment::new(RObject::view(env.find(".__enclos_env__")?)); |
There was a problem hiding this comment.
Could you change all these view() to new() please? It's too much work to assess safety. In general view() should only be used for dispatch.
crates/ark/src/variables/variable.rs
Outdated
|
|
||
| EnvironmentVariableNode::VectorElement { | ||
| match r_typeof(*object) { |
There was a problem hiding this comment.
While you're in there, could you please change all *obj to the clearer obj.sexp?
crates/ark/src/variables/variable.rs
Outdated
| let dim = IntegerVector::new(Rf_getAttrib(*object, R_DimSymbol))?; | ||
| let n_row = dim.get_unchecked(0).unwrap() as isize; | ||
|
|
||
| // TODO: use ? here, but this does not return a crate::error::Error, so |
There was a problem hiding this comment.
yep it's fine to return an Error::Anyhow variant.
crates/ark/src/variables/variable.rs
Outdated
| Some(value) => { | ||
| // Make sure value is a list before using inspect_list | ||
| if !r_typeof(value.sexp) == LISTSXP { | ||
| return Err(harp::Error::Anyhow(anyhow!( |
There was a problem hiding this comment.
Should this method return an anyhow::Result instead of harp::Result?
crates/ark/src/variables/variable.rs
Outdated
| let r_names = unsafe { RObject::new(Rf_getAttrib(value.sexp, R_NamesSymbol)) }; | ||
| if r_is_null(r_names.sexp) { | ||
| return vec![None; n]; | ||
| } | ||
|
|
||
| let names = unsafe { CharacterVector::new_unchecked(r_names) }; |
There was a problem hiding this comment.
Simplify with RObject::attr() or RObject::names()?
crates/ark/src/variables/variable.rs
Outdated
| let list = List::new(value.sexp)?; | ||
| let n = unsafe { list.len() }; | ||
|
|
||
| let names = local!({ |
dfalbel
force-pushed
the
feature/extend-variables-inspect2
branch
from
24172d0 to
22490ec
Compare
This a follow up for #560. See there for more details.
This PR adds support for custom inspect behavior using the
ark_variable_get_child()andark_variable_get_children()methods. It's made in a separate PR as the large diff is confusing to review. Most of it though, is a refactor to make the recursive behavior ofresolve_object_from_path()more explicit.