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

let start open anything and everything #7580

Merged
merged 5 commits into from Jan 3, 2023
Merged

Conversation

merelymyself
Copy link
Contributor

Description

Fixes #7546 's request. I'm unsure, so hopefully someone in charge of design can chip in.

User-Facing Changes

open now opens directories in the default file manager.

Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

@rgwood
Copy link
Contributor

rgwood commented Dec 23, 2022

Thanks for the PR. I don't think we should add this functionality to open - I don't like the idea of giving 1 command 2 very different jobs.

IIRC Nu used to have this functionality in a start plugin but that didn't get ported for the v0.60 rewrite. It used this crate: https://github.com/Byron/open-rs

We might be open to bringing this functionality back but I don't think it should happen in open.

@fdncred
Copy link
Collaborator

fdncred commented Dec 23, 2022

ya, I'd support a start plugin like we had before.

@rgwood
Copy link
Contributor

rgwood commented Dec 23, 2022

@fdncred do you prefer a plugin to a built-in command?

@fdncred
Copy link
Collaborator

fdncred commented Dec 23, 2022

@rgwood my tendency is to go with a plugin because that's what we had before. someone made that decision, i don't remember who. but it wouldn't hurt my feelings to have it built-in either.

@rgwood
Copy link
Contributor

rgwood commented Dec 23, 2022

I think I’d prefer a built-in command; the impact on compile time and binary size should be negligible.

@rgwood rgwood closed this Dec 23, 2022
@rgwood
Copy link
Contributor

rgwood commented Dec 23, 2022

Sorry, pressed the wrong button on my phone and closed this a bit too early.

@rgwood rgwood reopened this Dec 23, 2022
@fdncred
Copy link
Collaborator

fdncred commented Dec 23, 2022

@rgwood I would want to use the original crate though. It does quite a bit more checking and looks like it makes pretty good decisions per platform.

@merelymyself merelymyself changed the title let open open directories let open-dir open directories Dec 29, 2022
}
}

fn examples(&self) -> Vec<nu_protocol::Example> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These examples look like they're for open vs open-dir

@fdncred
Copy link
Collaborator

fdncred commented Dec 29, 2022

Not sure how I feel about open-dir vs start. I could probably live with it if others can since it says exactly what it's supposed to do where start is kind of ambiguous.

@rgwood
Copy link
Contributor

rgwood commented Dec 29, 2022

I don’t like the name open-dir because the command should be able to open files as well as directories.

@merelymyself merelymyself reopened this Dec 29, 2022
@merelymyself merelymyself marked this pull request as draft December 29, 2022 16:24
@fdncred
Copy link
Collaborator

fdncred commented Dec 30, 2022

I don’t like the name open-dir

Does that mean your proposal is to rename it to start, like it was before, or did you have something else in mind?

@rgwood
Copy link
Contributor

rgwood commented Dec 30, 2022

I think start is a good name but it is a little Windows-specific; I'd be open to other suggestions.

@merelymyself merelymyself marked this pull request as ready for review January 3, 2023 15:30
@merelymyself merelymyself changed the title let open-dir open directories let start open anything and everything Jan 3, 2023
Copy link
Collaborator

@fdncred fdncred left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

Copy link
Contributor

@rgwood rgwood left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@rgwood rgwood merged commit 6862734 into nushell:main Jan 3, 2023
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.

Add open . command works for opening directory
3 participants