Always display NA as unquoted in the Variables pane #432
Conversation
|
Thanks, then I mark this as ready. One note is that I'm wondering if |
There was a problem hiding this comment.
These options were introduced in order to be able to control the formatting for the data explorer (#344), but we no longer use the FormattedVector interface (see #382) because this interface wasn't flexible enough to support both use cases. AFAICT FormattedVector is currently only used by the Variables pane, so we can make it specific for it.
Another approach could be to simply remove those and we can add it back if we need it at some point.
We should also add a test for the NA formatting here to make sure we don't regress on it:
crates/harp/src/vector/mod.rs
Outdated
| fn format_elt_unchecked( | ||
| &self, | ||
| index: isize, | ||
| options: Option<&FormattedVectorCharacterOptions>, |
There was a problem hiding this comment.
I think we should move the definition of FormattedVectorCharacterOptions into this file.
We probably also want to change it's name to something like FormatOptions as it's no longer specific to character vectors.
crates/harp/src/vector/mod.rs
Outdated
| None => String::from("NA"), | ||
| } | ||
| } | ||
|
|
||
| fn format_with_string_options( |
There was a problem hiding this comment.
I think this should be renamed to format_with_options.
|
Thanks for the review! Renamed the struct and add a test. Also, thanks for the context. So, maybe it needs some more cleanup? I want to keep this pull request small for now (mainly because I don't know well about the implementations), but I can address in a separate pull request. |
There was a problem hiding this comment.
Thanks @yutannihilation for the PR! I have just added a few more comments, but this looks great!
crates/harp/src/vector/mod.rs
Outdated
| None => String::from("NA"), | ||
| } | ||
| } | ||
|
|
||
| fn format_with_options(&self, value: String, options: &FormatOptions) -> String { |
There was a problem hiding this comment.
If this only affects the character vectors, this should be moved to the character_vector.rs file.
| // Formatting options for character vectors | ||
| pub struct FormatOptions { | ||
| // Wether to quote the strings or not (defaults to `true`) | ||
| // If `true`, elements will be quoted during format so, eg: c("a", "b") becomes ("\"a\"", "\"b\"") in Rust |
There was a problem hiding this comment.
We should document that the quote option only affects character vectors.
|
Thanks! I inlined |
This pull request addresses posit-dev/positron#3810
Since
format()andformat_one()belongs toVector, I thinkformat_with_string_options()should also belong toVectorand be called there. But, I'm not sure if it's a good idea to includeFormattedVectorCharacterOptionsin the signature of all types ofVector. Considering posit-dev/positron#2860 will introduce a facility to handle this kind of distinction easier, I'm not sure if this is worth, but I share this code just in case this helps.