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

fix to pass EDITOR command with args #10

Merged
merged 2 commits into from
Sep 18, 2020
Merged

Conversation

tgfjt
Copy link
Contributor

@tgfjt tgfjt commented Sep 15, 2020

Hi 😸 I like mmv! Thank you for your creativity.

  1. I wanted to set $EDITOR = vscode
  2. but needed to wait the vscode instance finished
  3. and then I set $EDITOR with args. this below:
export EDITOR="code --wait" # same error occurs w/ "vi -e" 

mmv *.svg
mmv: abort renames: exec: "code --wait": executable file not found in $PATH

I fixed to pass $EDITOR with args to exec.Command.
What do you think of the approach?

Thanks!

@itchyny
Copy link
Owner

itchyny commented Sep 15, 2020

It look cool, but how is it common or standard to do this? Apparently zsh does not support this (this is about variable expansion) and crontab on macOS does not.

 % export EDITOR="vi -e"
 % crontab -e
crontab: vi -e: No such file or directory

Maybe it's not supported in other applications too.

@tgfjt
Copy link
Contributor Author

tgfjt commented Sep 15, 2020

Exactly. I'm not sure that.
I only saw it git commit is wating for $EDITOR code --wait.
https://github.com/git/git/blob/master/editor.c#L63-L77

I try to look at other things.:)

@tgfjt
Copy link
Contributor Author

tgfjt commented Sep 18, 2020

I made sure of edit command in mysql-shell is also waiting code --wait.
(edit is new for me, never used this command before... )
https://github.com/mysql/mysql-shell/blob/master/src/mysqlsh/commands/command_edit.cc#L99

cmd/mmv/main.go Outdated
@@ -100,7 +100,11 @@ func rename(args []string) error {
return err
}
defer tty.Close()
cmd := exec.Command(editor, f.Name())

editorWithArgs := strings.Split(editor, " ")
Copy link
Owner

Choose a reason for hiding this comment

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

Please change this split to strings.Fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 fixed fce8377

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't knew strings.Fields. Thanks!

Copy link
Owner

@itchyny itchyny left a comment

Choose a reason for hiding this comment

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

LGTM

@itchyny itchyny merged commit 485dd72 into itchyny:master Sep 18, 2020
@itchyny
Copy link
Owner

itchyny commented Sep 19, 2020

Released as v0.1.2.

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

2 participants