Data Explorer: Add support for date summary stats #388
Conversation
dfalbel
force-pushed
the
feature/date-stats
branch
from
3bcd7ff to
0e6345e
Compare
| Some(indices) => RFunction::from("col_filter_indices") | ||
| .add(column) | ||
| .add(RObject::try_from(indices)?) | ||
| .call_in(ARK_ENVS.positron_ns)?, |
There was a problem hiding this comment.
Might be worth extracting this sort of calls to R in Rust functions for readability?
| number_summary_stats <- function(column, filtered_indices) { | ||
| col <- col_filter_indices(column, filtered_indices) | ||
|
|
||
| summary_stats_number <- function(col) { |
There was a problem hiding this comment.
I like the new naming scheme.
| } | ||
|
|
||
| fn robj_to_string(robj: &RObject) -> String { | ||
| let string: Option<String> = unwrap!(robj.clone().try_into(), Err(e) => { |
There was a problem hiding this comment.
Add a conversion method for &RObject? The moving one could call the borrowing one.
There was a problem hiding this comment.
We actually don't need this any longer: 75dd37b
|
|
||
| fn robj_to_string(robj: &RObject) -> String { | ||
| let string: Option<String> = unwrap!(robj.clone().try_into(), Err(e) => { | ||
| log::error!("Date stats: Error converting RObject to String: {e}"); |
There was a problem hiding this comment.
Why are errors here logged instead of being propagated like the other errors.? If intended, probably worth a comment?
| .call_in(ARK_ENVS.positron_ns)?) | ||
| } | ||
|
|
||
| macro_rules! impl_summary_stats_conversion { |
There was a problem hiding this comment.
Is this macro necessary? It obfuscates what's going on quite a lot.
There was a problem hiding this comment.
You are right, I think this is simpler: c191c3a
Co-authored-by: Lionel Henry <[email protected]>
dfalbel
force-pushed
the
feature/date-stats
branch
from
75dd37b to
c03486c
Compare
This also refactors the summary stats support so it's easier to test it idependently.
Addresses posit-dev/positron#2161