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

Easier "Initialization" #46

Open
0x5a4 opened this issue Jul 20, 2022 · 17 comments
Open

Easier "Initialization" #46

0x5a4 opened this issue Jul 20, 2022 · 17 comments

Comments

@0x5a4
Copy link

0x5a4 commented Jul 20, 2022

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
Copy link
Owner

mgunyho commented Jul 21, 2022

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
Copy link
Contributor

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:

  • The location of tere needs to be specified, because as written this wrapper will not work. Running tere init bash could print out this wrapper with /path/to/tere replaced with the actual path to tere. This would minimize confusion.
  • Many others and myself use dotfiles to manage things like shell configurations across multiple computers. tere might be located in different places on different computers, which would make it impossible to use this wrapper. If the correct path is automatically filled in by running eval "$(tere init bash)", this problem won't occur.
  • Having each shell wrapper setup be only one line is cleaner and is what most other tools that are like tere do. If people want to see what the wrapper does, they can run tere init bash and inspect the shell script themselves.
  • If any changes are ever made to any of the shell wrappers, none of the old wrappers will break if they use eval "$(tere init bash)", but if they just copied it from the README the old wrappers will break and will need to be manually updated. Since wrappers will almost certainly have to had changed made to them, this is an important improvement to make.

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
Copy link
Author

0x5a4 commented Jul 21, 2022

@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
Copy link
Contributor

superatomic commented Jul 21, 2022

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
Copy link
Author

0x5a4 commented Jul 21, 2022

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
Copy link
Contributor

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
Copy link
Author

0x5a4 commented Jul 21, 2022

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
Copy link
Contributor

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
Copy link
Author

0x5a4 commented Jul 21, 2022

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
Copy link
Contributor

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:

  • Add a tere init command to print shell wrappers.
  • Add a configuration file in some format (TOML?) and a TUI configuration helper to modify the configuration file.

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
Copy link
Author

0x5a4 commented Jul 21, 2022

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
Copy link
Contributor

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
Copy link
Owner

mgunyho commented Aug 2, 2022

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.

  • 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.
  • If you feel that the shell wrapper is cluttering up your rc file, you can create a separate script for it and just source that. This way it can also act as a "config file".
  • tere will always just print the cwd on exit. So far there has been basically no need to update the shell wrappers. If it turns out that we need to change them often, maybe the one-liner starts to make more sense.
  • Source/eval needs extra escaping of quotes, like if --history-file contains spaces, and likely when binding custom keyboard shortcuts once that is implemented.
  • The starship one-liner for cmd requires installing an extra program for running lua scripts
  • 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.

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
Copy link
Owner

mgunyho commented Aug 2, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Owner

mgunyho commented Aug 4, 2022

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?

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

No branches or pull requests

3 participants