-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Mime command #12695
base: main
Are you sure you want to change the base?
Mime command #12695
Conversation
Should this start its life out as a plugin? Open question for me around #12657
|
I think the PR is mostly done now. As for your question, you're right that anything you can ask about this can also be asked about Regarding where we get mime associations, I don't think the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it were just up to me, who favors predictability/long-term maintenance over some features, I would advocate for the removal of ls --mime-type
and see the replacement in your commands shipped as a plugin, until there is a overwhelming need and proven reliability of the implementation.
But others may have different priorities and can resolve the stability/platform-support questions.
result: Some(Value::list( | ||
vec![ | ||
Value::record( | ||
record!("name" => Value::string("video.mkv".to_string(), NO_SPAN), "type" => Value::string("video/x-matroska", NO_SPAN)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get rid of all the NO_SPAN
s by using the Value::test_...
constructors. If I recall we don't need to explicitly call .to_string
either for those.
let guess_function: fn(&str) -> mime_guess::MimeGuess = if use_extension { | ||
mime_guess::from_ext | ||
} else { | ||
// HACK I don't know how to satisfy the compiler here without a closure, but I cannot return the function directly. | ||
// If I do, I get an error that the types are different or that a value does not live long enough when the function is called. | ||
|input| mime_guess::from_path(input) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that is due to the generic P: AsRef<Path>
on from_path
compared to the concrete &str
parameter in from_ext
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the genericness of from_path
is not playing nice with the concreteness of from_ext
. That's the reason for the error that the types mismatch. That's why I try to coerce it into the same type with a closure. Hopefully the compiler will optimize that away.
However, another error shows up later when I use the variable: guess_function
does not live long enough. I'm not sure why that one is caused. Isn't it supposed to be a cheaply Copy
-able fn pointer? Either way the closure solves that too.
Err(err) => Value::error( | ||
ShellError::TypeMismatch { | ||
err_message: err.to_string(), | ||
span, | ||
}, | ||
span, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to_string
ing the whole original error seems wrong. Either simply keep the ShellError::CantConvert
from Value::as_str
or do a proper ShellError::OnlySupportsThisInputType
Personally I'd prefer these commands to be builtin cause that makes them easier to use and discover, but I'm already making them for a probably niche use-case so ultimately I wouldn't mind if they were a plugin either. I'll leave that decision to the Nu higher-ups. |
I must emphasize, however, that I've already mentioned this behavior in the usage part of |
There are a lot of commands that I could live with as plugins, this is one of them. I'm pretty sure that I wrote the The thing that I always come back to is that nushell is a "batteries included" shell. If we remove all the batteries, it's not as well rounded and helpful but at what point are there enough batteries? I'm not sure. |
Okay, I'll rewrite this to be a plugin soon. I'll make this into a draft again until it's ready. If that's not okay you can go ahead and close this PR and I'll remake it with the changes. |
I'm going to need some help @fdncred. Since I want fn run(
&self,
plugin: &Self::Plugin,
engine: &nu_plugin::EngineInterface,
call: &nu_plugin::EvaluatedCall,
input: PipelineData,
) -> Result<nu_protocol::PipelineData, nu_protocol::LabeledError> {
let ctrlc = todo!(); // what to do here?
Ok(mime_records_iter.into_pipeline_data(call.head, ctrlc))
} |
I'm not sure ctrlc is exposed to plugins yet. Is it @devyn? |
Alright I'll push what I have so far anyway. It might make it easier to see what I'm trying to do. |
crates/nu_plugin_mime/src/guess.rs
Outdated
@@ -124,15 +131,16 @@ impl Command for MimeGuess { | |||
} | |||
}); | |||
|
|||
let ctrlc = engine_state.ctrlc.clone(); | |||
let ctrlc = compile_error!("can't figure out how to get ctrlc in plugin yet"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the part in question where I need ctrlc to be able to make this streamable.
Hello, any news on this @fdncred? |
Which part? We decided it needs to be a plugin. I don't think we have resolved the ctrlc part for plugins. |
That's the part. Any news about ctrlc for plugins? |
Hi @kik4444 It's not necessary to pass ctrlc to make a stream for a plugin. The engine side adds it for you, and handles the messages to cancel the stream on the plugin side. You can always just pass |
Hello @devyn, Is there an issue in the way nushell passes > glob * | mime guess
Plugin `mime` error: Plugin failed to decode: missing field `span`
Error: × Failed to receive response to plugin call from `mime`
╭─[entry #18:1:10]
1 │ glob * | mime guess
· ─────┬────
· ╰── while waiting for this operation to complete
╰────
help: try restarting the plugin with `plugin use 'mime'` But on the other hand if I collect the stream into a list before piping it then there's no problem. glob * | collect {} | mime guess |
Hi @kik4444, I believe your |
You were right. Now it seems to work as far as I can tell. I think this is now ready for review. |
I think this should be in a separate repo and not in nushell. Then we can add it to the awesome-nu repo and people can use it. |
Description
Initial implementation of #12657.
Add a new dedicated series of commands for working with mime types without disk access.
mime
- simply a help command likestr
mime list
- returns list of extensions for the given mimemime guess
- returns mime type guesses for the given file path or extension. Input / Output isstring -> string
andlist<string> -> table<name: string, type: string>
I have not yet figured out how to work withPipelineData::ListStream
and until I do this will be a draft PR.Everything shown in the examples works.
I am not too familiar with the internals of the Nushell codebase, so please let me know if I've made any mistakes or inefficiencies, such as with the spans.
What feedback do you have for the current state of these commands?
User-Facing Changes
New commands for working with mime types. Here's a sample from the help pages:
Tests + Formatting
cargo clippy --workspace -- -D warnings -D clippy::unwrap_used
currently fails because of an unused variable in thePipelineData::ListStream
section.After Submitting