extern/nushell
1
0
Fork 1
mirror of https://github.com/nushell/nushell.git synced 2024-11-25 01:43:47 +01:00
Code Issues Packages Projects Releases Wiki Activity

Improve handling of custom values in plugin examples (#12409)

Browse Source 
# Description
Requested by @ayax79. This makes the custom value behavior more correct,
by calling the methods on the plugin to handle the custom values in
examples rather than the methods on the custom values themselves. This
helps for handle-type custom values (like what he's doing with
dataframes).

- Equality checking in `PluginTest::test_examples()` changed to use
`PluginInterface::custom_value_partial_cmp()`
- Base value rendering for `PluginSignature` changed to use
`Plugin::custom_value_to_base_value()`
- Had to be moved closer to `serve_plugin` for this reason, so the test
for writing signatures containing custom values was removed
- That behavior should still be tested to some degree, since if custom
values are not handled, signatures will fail to parse, so all of the
other tests won't work.

# User-Facing Changes

- `Record::sort_cols()` method added to share functionality required by
`PartialCmp`, and it might also be slightly faster
- Otherwise, everything should mostly be the same but better. Plugins
that don't implement special handling for custom values will still work
the same way, because the default implementation is just a pass-through
to the `CustomValue` methods.

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`
This commit is contained in:
Devyn Cairns 2024-04-05 19:57:20 -07:00 committed by GitHub
parent c82dfce246
commit 2562e306b6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 178 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

82
crates/nu-plugin-test-support/src/plugin_test.rs
View File

@ -1,4 +1,4 @@
use std::{convert::Infallible, sync::Arc};
use std::{cmp::Ordering, convert::Infallible, sync::Arc};
use nu_ansi_term::Style;
use nu_engine::eval_block;
@ -222,7 +222,7 @@ impl PluginTest {
});
// Check for equality with the result
if *expectation != value {
if !self.value_eq(expectation, &value)? {
// If they're not equal, print a diff of the debug format
let expectation_formatted = format!("{:#?}", expectation);
let value_formatted = format!("{:#?}", value);
@ -272,4 +272,82 @@ impl PluginTest {
) -> Result<(), ShellError> {
self.test_examples(&command.examples())
}
/// This implements custom value comparison with `plugin.custom_value_partial_cmp()` to behave
/// as similarly as possible to comparison in the engine.
///
/// NOTE: Try to keep these reflecting the same comparison as `Value::partial_cmp` does under
/// normal circumstances. Otherwise people will be very confused.
fn value_eq(&self, a: &Value, b: &Value) -> Result<bool, ShellError> {
match (a, b) {
(Value::Custom { val, .. }, _) => {
// We have to serialize both custom values before handing them to the plugin
let mut serialized =
PluginCustomValue::serialize_from_custom_value(val.as_ref(), a.span())?;
serialized.set_source(Some(self.source.clone()));
let mut b_serialized = b.clone();
PluginCustomValue::serialize_custom_values_in(&mut b_serialized)?;
PluginCustomValue::add_source_in(&mut b_serialized, &self.source)?;
// Now get the plugin reference and execute the comparison
let persistent = self.source.persistent(None)?.get_plugin(None)?;
let ordering = persistent.custom_value_partial_cmp(serialized, b_serialized)?;
Ok(matches!(
ordering.map(Ordering::from),
Some(Ordering::Equal)
))
}
// All container types need to be here except Closure.
(Value::List { vals: a_vals, .. }, Value::List { vals: b_vals, .. }) => {
// Must be the same length, with all elements equivalent
Ok(a_vals.len() == b_vals.len() && {
for (a_el, b_el) in a_vals.iter().zip(b_vals) {
if !self.value_eq(a_el, b_el)? {
return Ok(false);
}
}
true
})
}
(Value::Record { val: a_rec, .. }, Value::Record { val: b_rec, .. }) => {
// Must be the same length
if a_rec.len() != b_rec.len() {
return Ok(false);
}
// reorder cols and vals to make more logically compare.
// more general, if two record have same col and values,
// the order of cols shouldn't affect the equal property.
let mut a_rec = a_rec.clone();
let mut b_rec = b_rec.clone();
a_rec.sort_cols();
b_rec.sort_cols();
// Check columns first
for (a, b) in a_rec.columns().zip(b_rec.columns()) {
if a != b {
return Ok(false);
}
}
// Then check the values
for (a, b) in a_rec.values().zip(b_rec.values()) {
if !self.value_eq(a, b)? {
return Ok(false);
}
}
// All equal, and same length
Ok(true)
}
(Value::Range { val: a_rng, .. }, Value::Range { val: b_rng, .. }) => {
Ok(a_rng.inclusion == b_rng.inclusion
&& self.value_eq(&a_rng.from, &b_rng.from)?
&& self.value_eq(&a_rng.to, &b_rng.to)?
&& self.value_eq(&a_rng.incr, &b_rng.incr)?)
}
// Must collect lazy records to compare.
(Value::LazyRecord { val: a_val, .. }, _) => self.value_eq(&a_val.collect()?, b),
(_, Value::LazyRecord { val: b_val, .. }) => self.value_eq(a, &b_val.collect()?),
// Fall back to regular eq.
_ => Ok(a == b),
}
}
}

36
crates/nu-plugin/src/plugin/command.rs
View File

@ -1,5 +1,6 @@
use nu_protocol::{
Example, LabeledError, PipelineData, PluginExample, PluginSignature, Signature, Value,
Example, IntoSpanned, LabeledError, PipelineData, PluginExample, PluginSignature, ShellError,
Signature, Value,
};
use crate::{EngineInterface, EvaluatedCall, Plugin};
@ -359,3 +360,36 @@ pub fn create_plugin_signature(command: &(impl PluginCommand + ?Sized)) -> Plugi
.collect(),
)
}
/// Render examples to their base value so they can be sent in the response to `Signature`.
pub(crate) fn render_examples(
plugin: &impl Plugin,
engine: &EngineInterface,
examples: &mut [PluginExample],
) -> Result<(), ShellError> {
for example in examples {
if let Some(ref mut value) = example.result {
value.recurse_mut(&mut |value| {
let span = value.span();
match value {
Value::Custom { .. } => {
let value_taken = std::mem::replace(value, Value::nothing(span));
let Value::Custom { val, .. } = value_taken else {
unreachable!()
};
*value =
plugin.custom_value_to_base_value(engine, val.into_spanned(span))?;
Ok::<_, ShellError>(())
}
// Collect LazyRecord before proceeding
Value::LazyRecord { ref val, .. } => {
*value = val.collect()?;
Ok(())
}
_ => Ok(()),
}
})?;
}
}
Ok(())
}

10
crates/nu-plugin/src/plugin/interface/engine.rs
View File

@ -403,16 +403,8 @@ impl EngineInterface {
/// Any custom values in the examples will be rendered using `to_base_value()`.
pub(crate) fn write_signature(
&self,
mut signature: Vec<PluginSignature>,
signature: Vec<PluginSignature>,
) -> Result<(), ShellError> {
// Render any custom values in the examples to plain values so that the engine doesn't
// have to keep custom values around just to render the help pages.
for sig in signature.iter_mut() {
for value in sig.examples.iter_mut().flat_map(|e| e.result.as_mut()) {
PluginCustomValue::render_to_base_value_in(value)?;
}
}
let response = PluginCallResponse::Signature(signature);
self.write(PluginOutput::CallResponse(self.context()?, response))?;
self.flush()

45
crates/nu-plugin/src/plugin/interface/engine/tests.rs
View File

@ -11,7 +11,7 @@ use crate::{
};
use nu_protocol::{
engine::Closure, Config, CustomValue, IntoInterruptiblePipelineData, LabeledError,
PipelineData, PluginExample, PluginSignature, ShellError, Signature, Span, Spanned, Value,
PipelineData, PluginSignature, ShellError, Span, Spanned, Value,
};
use std::{
collections::HashMap,
@ -782,49 +782,6 @@ fn interface_write_signature() -> Result<(), ShellError> {
Ok(())
}
#[test]
fn interface_write_signature_custom_value() -> Result<(), ShellError> {
let test = TestCase::new();
let interface = test.engine().interface_for_context(38);
let signatures = vec![PluginSignature::new(
Signature::build("test command"),
vec![PluginExample {
example: "test command".into(),
description: "a test".into(),
result: Some(Value::test_custom_value(Box::new(
expected_test_custom_value(),
))),
}],
)];
interface.write_signature(signatures.clone())?;
let written = test.next_written().expect("nothing written");
match written {
PluginOutput::CallResponse(id, response) => {
assert_eq!(38, id, "id");
match response {
PluginCallResponse::Signature(sigs) => {
assert_eq!(1, sigs.len(), "sigs.len");
let sig = &sigs[0];
assert_eq!(1, sig.examples.len(), "sig.examples.len");
assert_eq!(
Some(Value::test_int(expected_test_custom_value().0 as i64)),
sig.examples[0].result,
);
}
_ => panic!("unexpected response: {response:?}"),
}
}
_ => panic!("unexpected message written: {written:?}"),
}
assert!(!test.has_unconsumed_write());
Ok(())
}
#[test]
fn interface_write_engine_call_registers_subscription() -> Result<(), ShellError> {
let mut manager = TestCase::new().engine();

9
crates/nu-plugin/src/plugin/mod.rs
View File

@ -33,8 +33,8 @@ use std::os::unix::process::CommandExt;
#[cfg(windows)]
use std::os::windows::process::CommandExt;
use self::gc::PluginGc;
pub use self::interface::{PluginRead, PluginWrite};
use self::{command::render_examples, gc::PluginGc};
mod command;
mod context;
@ -651,7 +651,12 @@ where
let sigs = commands
.values()
.map(|command| create_plugin_signature(command.deref()))
.collect();
.map(|mut sig| {
render_examples(plugin, &engine, &mut sig.examples)?;
Ok(sig)
})
.collect::<Result<Vec<_>, ShellError>>()
.try_to_report(&engine)?;
engine.write_signature(sigs).try_to_report(&engine)?;
}
// Run the plugin on a background thread, handling any input or output streams

5
crates/nu-plugin/src/plugin/source.rs
View File

@ -40,7 +40,10 @@ impl PluginSource {
/// Try to upgrade the persistent reference, and return an error referencing `span` as the
/// object that referenced it otherwise
pub(crate) fn persistent(&self, span: Option<Span>) -> Result<Arc<dyn GetPlugin>, ShellError> {
///
/// This is not a public API.
#[doc(hidden)]
pub fn persistent(&self, span: Option<Span>) -> Result<Arc<dyn GetPlugin>, ShellError> {
self.persistent
.upgrade()
.ok_or_else(|| ShellError::GenericError {

8
crates/nu-plugin/src/protocol/plugin_custom_value.rs
View File

@ -224,7 +224,7 @@ impl PluginCustomValue {
/// Serialize a custom value into a [`PluginCustomValue`]. This should only be done on the
/// plugin side.
pub(crate) fn serialize_from_custom_value(
pub fn serialize_from_custom_value(
custom_value: &dyn CustomValue,
span: Span,
) -> Result<PluginCustomValue, ShellError> {
@ -240,7 +240,7 @@ impl PluginCustomValue {
/// Deserialize a [`PluginCustomValue`] into a `Box<dyn CustomValue>`. This should only be done
/// on the plugin side.
pub(crate) fn deserialize_to_custom_value(
pub fn deserialize_to_custom_value(
&self,
span: Span,
) -> Result<Box<dyn CustomValue>, ShellError> {
@ -272,7 +272,7 @@ impl PluginCustomValue {
///
/// This method will collapse `LazyRecord` in-place as necessary to make the guarantee,
/// since `LazyRecord` could return something different the next time it is called.
pub(crate) fn verify_source(
pub fn verify_source(
value: Spanned<&dyn CustomValue>,
source: &PluginSource,
) -> Result<(), ShellError> {
@ -356,7 +356,7 @@ impl PluginCustomValue {
}
/// Render any custom values in the `Value` using `to_base_value()`
pub(crate) fn render_to_base_value_in(value: &mut Value) -> Result<(), ShellError> {
pub fn render_to_base_value_in(value: &mut Value) -> Result<(), ShellError> {
value.recurse_mut(&mut |value| {
let span = value.span();
match value {

33
crates/nu-protocol/src/value/mod.rs
View File

@ -2441,15 +2441,28 @@ impl PartialOrd for Value {
// reorder cols and vals to make more logically compare.
// more general, if two record have same col and values,
// the order of cols shouldn't affect the equal property.
let (lhs_cols_ordered, lhs_vals_ordered) = reorder_record_inner(lhs);
let (rhs_cols_ordered, rhs_vals_ordered) = reorder_record_inner(rhs);
let mut lhs = lhs.clone();
let mut rhs = rhs.clone();
lhs.sort_cols();
rhs.sort_cols();
let result = lhs_cols_ordered.partial_cmp(&rhs_cols_ordered);
if result == Some(Ordering::Equal) {
lhs_vals_ordered.partial_cmp(&rhs_vals_ordered)
} else {
result
// Check columns first
for (a, b) in lhs.columns().zip(rhs.columns()) {
let result = a.partial_cmp(b);
if result != Some(Ordering::Equal) {
return result;
}
}
// Then check the values
for (a, b) in lhs.values().zip(rhs.values()) {
let result = a.partial_cmp(b);
if result != Some(Ordering::Equal) {
return result;
}
}
// If all of the comparisons were equal, then lexicographical order dictates
// that the shorter sequence is less than the longer one
lhs.len().partial_cmp(&rhs.len())
}
Value::LazyRecord { val, .. } => {
if let Ok(rhs) = val.collect() {
@ -3766,12 +3779,6 @@ impl Value {
}
}
fn reorder_record_inner(record: &Record) -> (Vec<&String>, Vec<&Value>) {
let mut kv_pairs = record.iter().collect::<Vec<_>>();
kv_pairs.sort_by_key(|(col, _)| *col);
kv_pairs.into_iter().unzip()
}
// TODO: The name of this function is overly broad with partial compatibility
// Should be replaced by an explicitly named helper on `Type` (take `Any` into account)
fn type_compatible(a: Type, b: Type) -> bool {

26
crates/nu-protocol/src/value/record.rs
View File

@ -257,6 +257,32 @@ impl Record {
iter: self.inner.drain(range),
}
}
/// Sort the record by its columns.
///
/// ```rust
/// use nu_protocol::{record, Value};
///
/// let mut rec = record!(
/// "c" => Value::test_string("foo"),
/// "b" => Value::test_int(42),
/// "a" => Value::test_nothing(),
/// );
///
/// rec.sort_cols();
///
/// assert_eq!(
/// Value::test_record(rec),
/// Value::test_record(record!(
/// "a" => Value::test_nothing(),
/// "b" => Value::test_int(42),
/// "c" => Value::test_string("foo"),
/// ))
/// );
/// ```
pub fn sort_cols(&mut self) {
self.inner.sort_by(|(k1, _), (k2, _)| k1.cmp(k2))
}
}
impl FromIterator<(String, Value)> for Record {