toumorokoshi / tome

convert any directory of scripts into your own fully-featured command.
MIT License
38 stars 2 forks source link

Batch 1: Github Actions, Brew recipe, linting suggestions, fish completion etc #4

Closed zph closed 3 years ago

zph commented 3 years ago

Closes #3.

See #3 for starting context. Then I left inline github comments and resolved the ones that are informational only and left unresolved the ones that need and answer or change.

Changes:

TODO pre-merge:

TODO post-merge:

zph commented 3 years ago

In absence of CI:

image
zph commented 3 years ago

@toumorokoshi Ready for initial review while I work on finishing the TODOs 🥇

zph commented 3 years ago

Ok, in a clean shell, using local build, it's behaving well for fish shell & tests are still passing.

zph commented 3 years ago

re: SHELL variable... (aka https://github.com/toumorokoshi/tome/pull/4#discussion_r598067584)

I'd like your input. The constraint is: when initializing the tome init cmd ./example $0 that $0 only works in zsh and bash, not in fish. I built in the look at ENV var SHELL which is default set in terminal as a fallback so zsh, bash and fish could all try to use the same interface. Note that zsh and bash will not need to ever use the $SHELL fallback because they'll have $0 set.

$SHELL variable should be set in all terminals by default, without additional configuration. It's a builtin mechanism. You can check if it's set for you by opening a clean new terminal and echo $SHELL.

But... my preference is to require someone to explicitly declare their shell, like you're saying. This removes need for $SHELL fallback and makes the initialization command consistent over all shells, ie:

tome init cmd ./example [bash|zsh|fish]

There's precedent for requiring the manual declaration in tools like direnv's init interface :).

TLDR: I'll happily make it and docs match with having people declare what shell.

Does that work well for you?

toumorokoshi commented 3 years ago

There's precedent for requiring the manual declaration in tools like direnv's init interface :). TLDR: I'll happily make it and docs match with having people declare what shell. Does that work well for you?

Works for me! I don't think SHELL is reliable, as that just dictates the user's login shell, rather than the current working shell.

See the following from man login on Linux:

    The user and group ID will be set according to their values in the /etc/passwd file.  There is one exception if the user ID is zero: in this case, only the primary group ID of the account is set.  This should al‐
       low the system administrator to login even in case of network problems.  The value for $HOME, $USER, $SHELL, $PATH, $LOGNAME, and $MAIL are set according to the appropriate fields in the  password  entry.   $PATH
       defaults to /usr/local/bin:/bin:/usr/bin for normal users, and to /usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin for root, if not otherwise configured.

Add an example of SHELL misreporting on my machine:

tsutsumi:/home/tsutsumi (0)
$ /bin/bash
[tsutsumi@tsutsumi-laptop ~]$ echo $SHELL
/bin/zsh
zph commented 3 years ago

@toumorokoshi Ok("yay").

The tests are passing, the TODOs from this PR are either

Except removing the duplication in init.rs among the shells. I get stuck on type issues still. The closest I came was in 588bbd6 but then the {function_name} interpolation wasn't happening. I welcome your patch but I've beaten my novice rust chops against it unsuccessfully.

Aside from that:

:D I'm ready for you to take a final look through this and then we can merge. I think I'll have time in next 3 days to work on Batch #2 which is what I'm running for myself right now.

Can you please pull this down and try it out for yourself?