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

Editor shell command #336

Closed
xFA25E opened this issue Jul 25, 2022 · 7 comments · Fixed by #337
Closed

Editor shell command #336

xFA25E opened this issue Jul 25, 2022 · 7 comments · Fixed by #337
Labels
s: Client This issue touches the pueue client s: Daemon This issue touches pueue daemon s: Pueue-lib This issue touches the pueue-lib library t: Enhancement Something that exists, but could be made better

Comments

@xFA25E
Copy link

xFA25E commented Jul 25, 2022

Is your feature request related to a problem?

Yes. Many popular programs (like git or nix) treat $EDITOR as a shell command. PUEUE treats it as an executable. As a result, when I put myeditor --option=value in it, pueue edit fails to find it.

Describe the solution you'd like

I'd like to change that behaviour and treat $EDITOR as a shell command. I see 2 possible solutions.

1. Use shell-escape on file

Since PUEUE depends on shell-escape, we can just concatenate escaped file to $EDITOR and use something like compile_shell_command to run the editor.

2. Parse $EDITOR string

We can adopt a simple split on \n \t\r like nix does, for example.

Otherwise, if that seems too dangerous (it is, indeed), we can use a crate like shlex for parsing.

Describe alternatives you've considered

I can create executables for every different command-line option, I guess. It is particularly painful, when I need to change $EDITOR on the fly, like here: EDITOR="myeditor --flag" pueue edit <task-id>

Additional context

If you think that this feature is worth pursuing I can submit a PR, of course.

@Nukesor
Copy link
Owner

Nukesor commented Jul 27, 2022

I guess if the big guys do it that way, it wouldn't be a bad idea to do it as well :D.

From what I can see, the temporary filenames are always purely alphanumeric. We wouldn't even have to escape them:
https://github.com/Stebalien/tempfile/blob/master/src/util.rs#L12

One thing we should probably do, however, is to move the plattform specific daemon/plattform/process_helper module into pueue_lib.
That module contains all the duct-tape we need for platform agnostic shell handling.

And there's also one more thing we could think about while we're touching this part of the codebase:

I recently added a test-suite to properly test the client binary.
While doing so, I stumbled upon the editing logic and couldn't come up with a simple way of testing this (as it spawns a binary we would have to interact with).

With this new logic, we could maybe use a shell command which simply manipulates the file at the given path, instead of just spawning a binary.
This change could actually allow us to end-to-end test the whole renaming logic.

@Nukesor
Copy link
Owner

Nukesor commented Jul 27, 2022

In hindsight, I could have just placed a shell script in the temporary directory that does the exact same thing described above, which is then called as the $EDITOR.

The new solution feels much cleaner though.

@Nukesor
Copy link
Owner

Nukesor commented Jul 27, 2022

I'll start working on the migration of the platform specific module to pueue_lib.

@Nukesor
Copy link
Owner

Nukesor commented Jul 27, 2022

MR is out:
#337

This should enable us to do the whole shell handling quite easily.

@Nukesor
Copy link
Owner

Nukesor commented Jul 27, 2022

I actually finished the implementation for this feature request in said MR. It felt like a good fit to do everything in one go.

Could you go ahead and verify, that this now works as intended for your usecase?

cd /tmp
git clone git@github.com:Nukesor/pueue
cd pueue
git switch platform_specific_module_cleanup
cargo install --path . --locked

@Nukesor Nukesor added t: Enhancement Something that exists, but could be made better s: Pueue-lib This issue touches the pueue-lib library s: Daemon This issue touches pueue daemon s: Client This issue touches the pueue client labels Jul 27, 2022
@xFA25E
Copy link
Author

xFA25E commented Jul 28, 2022

Wow, that was fantastic! I tested it and it seems to be working great, thank you!

@Nukesor
Copy link
Owner

Nukesor commented Jul 28, 2022

Perfect :)

I'll probably wait a few days before publishing the next patch release as 2.1 is rather fresh, but I'll probably push a new release sometime next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: Client This issue touches the pueue client s: Daemon This issue touches pueue daemon s: Pueue-lib This issue touches the pueue-lib library t: Enhancement Something that exists, but could be made better
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants