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

Implement change_directory/show_directory commands #335

Merged
merged 2 commits into from
Jun 22, 2021

Conversation

simias
Copy link

@simias simias commented Jun 21, 2021

These commands let the user check and change the current working directory for the running helix session.

I mostly implemented those to play with the codebase but I thought they might be useful (they would be useful to me at least, I regularly change the working directory in vim).

As an aside, I think it would be nice if TypableCommands returned a Result<()> and any error was automatically logged using editor.set_error(_), that would let one bail! out of a command implementation without having to manually call editor.set_error(format!(...)); return;. I could submit such a change if it was deemed acceptable.

@archseer
Copy link
Member

Thanks for contributing!

As an aside, I think it would be nice if TypableCommands returned a Result<()> and any error was automatically logged using editor.set_error(_), that would let one bail! out of a command implementation without having to manually call editor.set_error(format!(...)); return;. I could submit such a change if it was deemed acceptable.

I think this would be a worthwhile change now that :commands are typed separately from regular commands. Most :commands should have some sort of output on error. You could probably define a new error type with thiserror similar to

#[error(transparent)]
Other(#[from] anyhow::Error),
for String, that way one could ? or return Err(string.into())

Copy link
Member

@CBenoit CBenoit 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 to me, thank you! I suggest changing just one little detail, but I'm not blocking. You also need to fix lints (see CI, but that's basically cargo clippy + cargo fmt).
Also, what about change-working-directory and show-working-directory for names?

As an aside, I think it would be nice if TypableCommands returned a Result<()> …

Agreed! Do you plan on doing a PR for that next?

helix-term/src/commands.rs Outdated Show resolved Hide resolved
@simias
Copy link
Author

simias commented Jun 22, 2021

Yeah I think I can submit a PR for the Result. At least if you're not in a hurry, I don't have a lot of time for hobby work these days...

@simias
Copy link
Author

simias commented Jun 22, 2021

Oh I just your point regarding the naming, I'm absolutely fine renaming them. For the record both Vim and Rust call it the "current directory", so an alternative could be show|change-current-directory but now we're seriously nearing bikeshed territory...

@CBenoit
Copy link
Member

CBenoit commented Jun 22, 2021

Yeah I think I can submit a PR for the Result. At least if you're not in a hurry, I don't have a lot of time for hobby work these days...

No hurry 🙂

Oh I just your point regarding the naming, I'm absolutely fine renaming them. For the record both Vim and Rust call it the "current directory", so an alternative could be show|change-current-directory but now we're seriously nearing bikeshed territory...

I'm fine with either one. Just take show|change-current-directory if it's used by VIM and Rust, and let's call it a day. 👍

@simias
Copy link
Author

simias commented Jun 22, 2021

Arg, I used the "working" version. Well let me amend it quickly.

Copy link
Member

@CBenoit CBenoit 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! Thank you!

@CBenoit CBenoit merged commit 16883e7 into helix-editor:master Jun 22, 2021
@simias simias deleted the dev/cd branch June 23, 2021 12:00
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.

3 participants