Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for all custom value operations on plugin custom values #12088

Merged
merged 7 commits into from Mar 12, 2024
Merged

Support for all custom value operations on plugin custom values #12088

merged 7 commits into from Mar 12, 2024

Conversation

devyn
Copy link
Contributor

@devyn devyn commented Mar 6, 2024 •
edited

Description

Adds support for the following operations on plugin custom values, in addition to to_base_value which was already present:

  • follow_path_int()
  • follow_path_string()
  • partial_cmp()
  • operation()
  • Drop (notification, if opted into with CustomValue::notify_plugin_on_drop)

There are additionally customizable methods within the Plugin and StreamingPlugin traits for implementing these functions in a way that requires access to the plugin state, as a registered handle model such as might be used in a dataframes plugin would.

Value::append was also changed to handle custom values correctly.

User-Facing Changes

  • Signature of CustomValue::follow_path_string and CustomValue::follow_path_int changed to give access to the span of the custom value itself, useful for some errors.
  • Plugins using custom values have to be recompiled because the engine will try to do custom value operations that aren't supported
  • Plugins can do more things 🎉

Tests + Formatting

Tests were added for all of the new custom values functionality.

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

  • Document protocol reference CustomValueOp variants:
    • FollowPathInt
    • FollowPathString
    • PartialCmp
    • Operation
    • Dropped
  • Document notify_on_drop optional field in PluginCustomValue

@devyn devyn force-pushed the plugin-custom-value-ops branch 2 times, most recently from 4b0b950 to 9e48391 Compare March 10, 2024 01:48
@devyn
Copy link
Contributor Author

devyn commented Mar 10, 2024

This should be good to go now - conflicts fixed, tests pass.

@devyn devyn marked this pull request as ready for review March 10, 2024 01:49
sholderbach
sholderbach reviewed Mar 10, 2024
View reviewed changes
Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to the cell path access sound good to me, more consistent and better errors are always appreciated.

Providing Drop notifications sounds good from a clean-up perspective. We should probably be clear about the fact that we can not guarantee the drop-order etc. for a while depending how engine internals evolve.

crates/nu-plugin/Cargo.toml Outdated Show resolved Hide resolved
crates/nu-plugin/src/protocol/plugin_custom_value.rs Outdated Show resolved Hide resolved
@devyn devyn mentioned this pull request Mar 12, 2024
Merged
2 tasks
@sholderbach sholderbach merged commit 73f3c0b into nushell:main Mar 12, 2024
16 checks passed
@hustcer hustcer added this to the v0.92.0 milestone Mar 13, 2024
devyn added a commit to devyn/nushell.github.io that referenced this pull request Mar 14, 2024
@devyn
Custom value ops documentation (nushell/nushell#12088)
e6c03b6
@devyn devyn mentioned this pull request Mar 14, 2024
Merged
fdncred pushed a commit to nushell/nushell.github.io that referenced this pull request Mar 14, 2024
@devyn
Custom value ops documentation (nushell/nushell#12088) (#1300)
c2031e9
fdncred pushed a commit that referenced this pull request Mar 14, 2024
@devyn
Reorganize plugin API around commands (#12170)
9cf2e87 
[Context on
Discord](https://discord.com/channels/601130461678272522/855947301380947968/1216517833312309419)

# Description
This is a significant breaking change to the plugin API, but one I think
is worthwhile. @ayax79 mentioned on Discord that while trying to start
on a dataframes plugin, he was a little disappointed that more wasn't
provided in terms of code organization for commands, particularly since
there are *a lot* of `dfr` commands.

This change treats plugins more like miniatures of the engine, with
dispatch of the command name being handled inherently, each command
being its own type, and each having their own signature within the trait
impl for the command type rather than having to find a way to centralize
it all into one `Vec`.

For the example plugins that have multiple commands, I definitely like
how this looks a lot better. This encourages doing code organization the
right way and feels very good.

For the plugins that have only one command, it's just a little bit more
boilerplate - but still worth it, in my opinion.

The `Box<dyn PluginCommand<Plugin = Self>>` type in `commands()` is a
little bit hairy, particularly for Rust beginners, but ultimately not so
bad, and it gives the desired flexibility for shared state for a whole
plugin + the individual commands.

# User-Facing Changes
Pretty big breaking change to plugin API, but probably one that's worth
making.

```rust
use nu_plugin::*;
use nu_protocol::{PluginSignature, PipelineData, Type, Value};

struct LowercasePlugin;
struct Lowercase;

// Plugins can now have multiple commands
impl PluginCommand for Lowercase {
    type Plugin = LowercasePlugin;

    // The signature lives with the command
    fn signature(&self) -> PluginSignature {
        PluginSignature::build("lowercase")
            .usage("Convert each string in a stream to lowercase")
            .input_output_type(Type::List(Type::String.into()), Type::List(Type::String.into()))
    }

    // We also provide SimplePluginCommand which operates on Value like before
    fn run(
        &self,
        plugin: &LowercasePlugin,
        engine: &EngineInterface,
        call: &EvaluatedCall,
        input: PipelineData,
    ) -> Result<PipelineData, LabeledError> {
        let span = call.head;
        Ok(input.map(move |value| {
            value.as_str()
                .map(|string| Value::string(string.to_lowercase(), span))
                // Errors in a stream should be returned as values.
                .unwrap_or_else(|err| Value::error(err, span))
        }, None)?)
    }
}

// Plugin now just has a list of commands, and the custom value op stuff still goes here
impl Plugin for LowercasePlugin {
    fn commands(&self) -> Vec<Box<dyn PluginCommand<Plugin=Self>>> {
        vec![Box::new(Lowercase)]
    }
}

fn main() {
    serve_plugin(&LowercasePlugin{}, MsgPackSerializer)
}
```

Time this however you like - we're already breaking stuff for 0.92, so
it might be good to do it now, but if it feels like a lot all at once,
it could wait.

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting
- [ ] Update examples in the book
- [x] Fix #12088 to match - this change would actually simplify it a
lot, because the methods are currently just duplicated between `Plugin`
and `StreamingPlugin`, but they only need to be on `Plugin` with this
change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sholderbach sholderbach sholderbach left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.92.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@devyn @sholderbach @hustcer