Raku / App-Rakubrew

Raku environment manager
https://rakubrew.org/
Other
26 stars 13 forks source link

Adds Ash, and tests for shells, closes #20 #21

Closed JJ closed 4 years ago

JJ commented 4 years ago

It basically takes into account that ash is unable to complete. Tested locally in a Docker environment, it seems to work. It might be convenient, however, to create some environment to tests different shell scripts using GitHub actions, for instance.

patrickbkr commented 4 years ago

The code for the Ash shell is basically identical to the fallback implementation in Sh.pm. I'm not sure there is much point then to have a separate implementation for Ash. Is there a specific reason why you deem a specific implementation for Ash necessary?

patrickbkr commented 4 years ago

Of note: The shell auto-detection in App::Rakubrew::Shell::detect_shell() falls back to the App::Rakubrew::Shell::Sh implementation if no other matches. That is what would / should happen for Ash.

Auto-detection happens when passing auto as the shell name. The basic idea is to use the explicit shell name in shell specific configuration files (like .zshrc or .bashrc) and auto in files which are read by multiple different shells (like .profile). There is still no guarantee that the wrong shell hook implementation will never be used (the $SHELL environment variable is sadly not reliable), but it helps.

JJ commented 4 years ago

Still, it's a different shell and it might drift in the future. And the tests will help anyway.

patrickbkr commented 4 years ago

I don't think it's a good idea to duplicate code only because it might possibly become useful in the future. Let's create a separate implementation for Ash once there is a necessity.

I just pushed a change to the installation instructions of the generic Sh shell hook to make it clearer that it works with all more or less posix compatible shells. I hope this reduces the confusion with this hook stating to only apply to "the sh shell".

I do like the tests. Could you change this PR to only contain those or create a new one?

JJ commented 4 years ago

I certainly can. Thanks.

patrickbkr commented 4 years ago

Looking good! Thanks for your work!