mgunyho / tere

Terminal file explorer
European Union Public License 1.2
1.68k stars 38 forks source link

Easier "Initialization" #46

Open 0x5a4 opened 2 years ago

0x5a4 commented 2 years ago

Something like source "$(tere --init shell)" similar to how starship does it? This could be nice to declutter one's bashrc(or whatever)? Would require either templating(see zoxide) or a config file though, since one could no longer configure the arguments otherwise... I'd be willing to implement both the printing, and either of the configuration solutions but am I overengineering this?

mgunyho commented 2 years ago

Thanks for the suggestion! This seems like a reasonable idea, especially if it is a requirement to get tere installable via homebrew (ref. #29).

However, I like that the current setup is explicit: it's pretty easy to understand what tere is doing and how it works, while this one-liner obscures is going on. You have to modify your shell rc file in any case, and I don't know if copy-pasting four lines instead of one is that much of an obstacle for first-time setup (which you have to do only once).

I would also like to avoid having a separate config file for now. tere has not that many settings, but having a config file is a big maintenance burden. If you have lots of options in the alias in your rc file, you can write a custom "config file" where you define the alias + options, and source it in your rc. (For example, nnn doesn't have a config file but instead uses environment variables for configuration. I would also look at how ripgrep decided on moving to a config file after initially only using flags, and see if the arguments apply here.)

superatomic commented 2 years ago

That makes sense. I wasn't suggesting a separate configuration file (maybe that should be split into a separate PR?), but I feel like it might be worth change the wrappers to be embedded in the client itself.

For example, take a look at the listed bash/zsh wrapper:

tere() {
    local result=$(/path/to/tere "$@")
    [ -n "$result" ] && cd -- "$result"
}

As it stands, I have a few problems with this wrapper:

In my opinion, these four reasons support implementing something like tere init <SHELL> to print shell wrappers for a specific shell.

Also, like I've said, I would be happy to put in the work to implement this feature myself and to submit a PR.

0x5a4 commented 2 years ago

@superatomic The config file would be required to retain functionality though, as there would no longer be a way to specify custom options.

Looking at e.g. zoxide this could also be solved using templating though, and the options could be the ones specified along the initial "call" to tere init.

But a config file might still be the better choice as it benefits future implementations of e.g. a config gui to be more newbie appealing. It could also allow auto installation of the shell scripts, similar to how broot does it(again avoiding someone uncomfortable with rc files to have to modify them).

I'd also be happy to implement it, as i am quite bored at the moment and have done similar before :)

Edit: Spelling

superatomic commented 2 years ago

Couldn't shell aliases just be used?

That, or any options provided to tere init could be inlined in the generated wrapper.

I don't believe a configuration file is required for this to work.

0x5a4 commented 2 years ago

Ok, but if we let users alias tere before the init function we pretty much blow up the purpose of the whole init thing. The inlining could be done using a template engine(e.g. tiny-template)

Im not sure if a Config GUI is planned but that feature totally requires a config file.

superatomic commented 2 years ago

Okay, so instead of aliases it could just be part of tere init. As an example, running tere init bash --mouse=on --folders-only would expand to this (if tere was located at /usr/local/bin):

tere() {
    local result=$(/usr/local/bin/tere --mouse=on --folders-only "$@")
    [ -n "$result" ] && cd -- "$result"
}

This would solve needing to use aliases.

0x5a4 commented 2 years ago

As this case is relatively simple a format! might also suffice to insert the arguments instead of using a template engine...

A config file would not be required for the init function but could be required in the future, making it wise to implement it now as it is one of the possible solutions to this problem as well.

Edit: My answer sounded way too passive aggressive, Clarification

superatomic commented 2 years ago

I see your point. If @mgunyho is okay with a configuration file (TOML, possibly?) I would be happy to implement that along with the init option, although it seems like that might come later (or never) as @mgunyho expressed that they didn't want to have a configuration file.

Luckily, the feature for a configuration file could always be added later and be fully backwards compatible if tere init is implemented.

0x5a4 commented 2 years ago

Yes, backwards compatibility would be given. But both the config file and the inlining of options kind of serve the same purpose: The user would still be able to configure tere, even though the actual invocation of it would be hidden away behind init. Except that the inlining can only be modified by the user and not the program itself...

Also I think that something like (broot's)[https://github.com/Canop/broot] auto-installation would be quite useful for command line newbies. This would then also require some kind of config GUI as having users edit an rc file would be just as "hard" as editing a config file.

superatomic commented 2 years ago

Do we want to split these off into separate issues? Even if both of them get implemented, they can both be implemented in any order, and neither depend on each other.

Basically, the issues are:

Not only would the second issue take more work, it might also come later or not at all (see @mgunyho's comments), although I do support it.

I think it would be more productive to discuss these improvements separately.

0x5a4 commented 2 years ago

Yeah you're right the config file doesnt belong here. What I meant to say is that their purpose overlaps, yet one is not as flexible as the other. So why not implement only one? Also the config file is basically done by deriving e.g. serde's Serialize, Deserialize on TereSettings...

superatomic commented 2 years ago

Yeah you're right the config file doesnt belong here. What I meant to say is that their purpose overlaps, yet one is not as flexible as the other. So why not implement only one? Also the config file is basically done by deriving e.g. serde's Serialize, Deserialize on TereSettings...

That's actually a lot simpler than I though it was going to be. I think you're right: all that needs to be done is to choose a file format (I vote TOML via toml-rs, but anything works), to add a few lines of code to load the configuration file, and to derive Serialize, Deserialize. I love Rust.

Of course, your idea for a TUI interface to the configuration file is a bit more complex.

I'd recommend that you open a separate issue and link to this issue as previous discussion, and then edit this issue to only discuss adding tere init. I think both can be worked on separately.

mgunyho commented 2 years ago

Sorry for the delayed response.

I did a (not very exhaustive or extensive) scan through some similar terminal apps to see how they handle this. They seem to roughly fall in to three categories:

  1. Manually copy-pasting shell config (like tere does now). This is done at least by nnn, lf, fff, clifm, joshuto, xplr, llama, autojump, fzf.
    • A variant of this is to have an option or auxiliary script that modifies the user's shell config instead of manual copy paste. Used by sdn, goto.
  2. One-liner with eval or source, like what is suggested here. This is used by zoxide, fasd, jump, z.sh, z.lua, starship, broot (also has an option to automatically modify shell config).
  3. The program is run (aliased) with source myprogram, and instead of printing the cwd, it prints a shell code snippet that will do the cd. This is pretty close to the second option. Used by cdir, bashmarks, deer, ranger (but can be made to work like the first option)

There doesn't seem to be a clear consensus on which is the best way.

Here are some thoughts I have related to the one-liner proposal.

Admittedly, these are mostly minor edge cases but I still would like to avoid creating them.

Sorry if I sound like I'm just arguing for the sake of it, I'm just a bit on the fence with this. It doesn't feel like the benefits would clearly outweigh the extra complexity and possibilities for bugs.

mgunyho commented 2 years ago

One option could be to just try to implement it and see how well it works, but we should be prepared to discard that work if I'm not happy with it.

superatomic commented 2 years ago

Here are some thoughts I have related to the one-liner proposal.

  • eval "$(tere init bash)" already assumes that tere is globally installed (be it via cargo install or homebrew). Under this assumption, the current shell wrappers can be made location-independent as well, with local result=$(command tere "$@"). This probably should be mentioned in the readme, I'll think about how to word it clearly. OTOH, if you install tere by manually downloading the binary, you will still have to explicitly write eval "$(/path/to/tere init bash)" and then the readme won't work "as written" either.

I like the idea of using command tere instead of /path/to/tere in the shell wrappers listed in the README, because it makes the shell wrappers just require a simple copy & paste and because it allows for using the wrapper even if tere is located in an arbitrary location. I would assume that most users of the software would have tere on their path. If you want to make sure people who don't have tere on their path don't get confused, a sentence explaining that could be added to the README.

  • The starship one-liner for cmd requires installing an extra program for running lua scripts

I don't believe anything like this would have to be done for tere.

  • The one-liner would hardcode the name of the wrapper function. This is maybe not so significant because a name collision is very unlikely, and the user can do an alias afterards.

The one-liner could accept an alias parameter to change the name of the command. The provided name would default to tere and could be filled in by using format!. For example, to have the wrapper just be named t:

eval "$(tere init --alias t)"
superatomic commented 2 years ago

Admittedly, these are mostly minor edge cases but I still would like to avoid creating them.

Sorry if I sound like I'm just arguing for the sake of it, I'm just a bit on the fence with this. It doesn't feel like the benefits would clearly outweigh the extra complexity and possibilities for bugs.

Don't worry about it! It's completely fair to be concerned about whether changes like this are worth doing (and if so, exactly how they should be implemented).

mgunyho commented 2 years ago

The one-liner could accept an alias parameter

This is fair. But note how we are now adding more complication.

Imagine the reverse situation, that the recommended way would be to have eval "$(tere init bash)" in your bashrc. Now, I would come around and tell you that by adding three lines to the recommended shell config, we could get rid of problems with escaping quotes, would make init and --alias options obsolete, and would also make it clearer to users what tere is doing under the hood (not that much!). Would that not sound pretty convincing?