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.

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

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Mime command #12695

wants to merge 13 commits into from

Conversation

kik4444
Copy link
Contributor

@kik4444 kik4444 commented Apr 28, 2024

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 like str
  • mime list - returns list of extensions for the given mime
  • mime guess - returns mime type guesses for the given file path or extension. Input / Output is string -> string and list<string> -> table<name: string, type: string>

I have not yet figured out how to work with PipelineData::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:

Get known extensions for the "video/x-matroska" mime type
  > mime list "video/x-matroska"
  ╭───┬──────╮
  │ 0 │ mk3d │
  │ 1 │ mks  │
  │ 2 │ mkv  │
  ╰───┴──────╯

  Get all known video extensions
  > mime list "video/*"
Guess the MIME type from the path and return a string.
  > "video.mkv" | mime guess
  "video/x-matroska"

  Guess the MIME types from the paths and return a table.
  > ["video.mkv" "audio.mp3"] | mime guess
  ╭───┬───────────┬──────────────────╮
  │ # │   name    │       type       │
  ├───┼───────────┼──────────────────┤
  │ 0 │ video.mkv │ video/x-matroska │
  │ 1 │ audio.mp3 │ audio/mpeg       │
  ╰───┴───────────┴──────────────────╯

  Guess the MIME types from the extensions and return a table.
  > ["mkv" "mp3"] | mime guess -e
  ╭───┬──────┬──────────────────╮
  │ # │ name │       type       │
  ├───┼──────┼──────────────────┤
  │ 0 │ mkv  │ video/x-matroska │
  │ 1 │ mp3  │ audio/mpeg       │
  ╰───┴──────┴──────────────────╯

  Add a MIME type column to a table.
  > let input = glob -d 1 * | wrap filename; $input | merge ($input | get filename | mime guess | select type)
  ╭───┬──────────┬──────╮
  │ # │ filename │ type │
  ├───┼──────────┼──────┤
  │ 0......  │
  ╰───┴──────────┴──────╯

Tests + Formatting

cargo clippy --workspace -- -D warnings -D clippy::unwrap_used currently fails because of an unused variable in the PipelineData::ListStream section.

After Submitting

@sholderbach
Copy link
Member

Should this start its life out as a plugin?

Open question for me around #12657

  • implementation cost seems low rn but is there a stability risk for nushell as the crate doesn't guarantee that the mime-type assignment is stable?
  • What is the right source of "truth" for mime type/extension mappings? A statically baked form in the binary, the system database like in /usr/share/mime?
    (the same discussion could apply to whether ls --mime-type is "stabilizable" or should overload the type column)

@kik4444 kik4444 marked this pull request as ready for review May 3, 2024 20:45
@kik4444
Copy link
Contributor Author

kik4444 commented May 3, 2024

Should this start its life out as a plugin?

Open question for me around #12657

* implementation cost seems low rn but is there a stability risk for nushell as the [crate doesn't guarantee that the mime-type assignment is stable](https://docs.rs/mime_guess/2.0.4/mime_guess/#note-mime-types-returned-are-not-stableguaranteed)?

* What is the right source of "truth" for mime type/extension mappings? A statically baked form in the binary, the system database like in `/usr/share/mime`?
  (the same discussion could apply to whether `ls --mime-type` is "stabilizable" or should overload the `type` column)

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 ls's --mime-type flag.

Regarding where we get mime associations, I don't think the mime_guess crate supports anything besides its own hardcoded associations. Though does every platform supported by Nushell even have something like a /usr/share/mime database?

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.

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)),
Copy link
Member

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_SPANs by using the Value::test_... constructors. If I recall we don't need to explicitly call .to_string either for those.

Comment on lines 105 to 111
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)
};
Copy link
Member

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?

Copy link
Contributor Author

@kik4444 kik4444 May 5, 2024

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.

Comment on lines 139 to 145
Err(err) => Value::error(
ShellError::TypeMismatch {
err_message: err.to_string(),
span,
},
span,
),
Copy link
Member

Choose a reason for hiding this comment

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

to_stringing the whole original error seems wrong. Either simply keep the ShellError::CantConvert from Value::as_str or do a proper ShellError::OnlySupportsThisInputType

@kik4444
Copy link
Contributor Author

kik4444 commented May 5, 2024

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.

@kik4444
Copy link
Contributor Author

kik4444 commented May 5, 2024

removal of ls --mime-type and see the replacement in your commands shipped as a plugin

I must emphasize, however, that ls --mime-type does more than my commands. My commands are a glorified wrapper over mime_guess. One notable difference is that ls checks if a path is a directory and then returns dir on the type column. This mime guess command does not perform disk access, so passing a name to it without an extension will return "unknown". And directories are all basically names without extensions.

I've already mentioned this behavior in the usage part of mime guess.

@fdncred
Copy link
Collaborator

fdncred commented May 7, 2024

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 ls --mime-type because someone asked for it, but for my usage, it is pretty niche.

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.

@kik4444
Copy link
Contributor Author

kik4444 commented May 8, 2024

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.

@kik4444 kik4444 marked this pull request as draft May 8, 2024 21:08
@kik4444
Copy link
Contributor Author

kik4444 commented May 11, 2024

I'm going to need some help @fdncred. Since I want mime guess to work with streams, I need to provide ctrlc to into_pipeline_data, but how do you get ctrlc inside a plugin? I don't see any fields or methods on engine: &nu_plugin::EngineInterface that can help and I can't find any examples of this elsewhere in the code.

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))
}

@fdncred
Copy link
Collaborator

fdncred commented May 11, 2024

I'm not sure ctrlc is exposed to plugins yet. Is it @devyn?

@kik4444
Copy link
Contributor Author

kik4444 commented May 12, 2024

Alright I'll push what I have so far anyway. It might make it easier to see what I'm trying to do.

@@ -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");
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants