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

ZSH and OS X compatibility #60

Merged
merged 4 commits into from Oct 22, 2012
Merged

ZSH and OS X compatibility #60

merged 4 commits into from Oct 22, 2012

Conversation

g5pw
Copy link
Contributor

@g5pw g5pw commented Oct 22, 2012

Hello!
This pull request addresses some minor issues with the shell_shortcuts.sh.

  • I am on OS X and use GNU coreutils, so I have GNU ls and readlink. I updated the code to check the actual commands (like, say, duck typing) and not architecture.
  • the ls_with_file_shortcuts function was failing due on ZSH due to missing sh_word_split option
  • ls_with_file_shortcuts was failing on ZSH due to eval interpreting the $_ll_command as a whole command, and of course a file named "ls -G" was not found in path :)

Please feel free to comment/amend my code as you will.

This patch modifies the way available commands are tested; If a Mac user installs and uses GNU ls it is correctly detected and used.
the ls_with_file_shortcuts now works on ZSH. It was failing with "command not found" due to parameters passed in $_ll_command which were interpreted as a command name.
This fixes ZSH compatibility by ensuring sh_word_split is on.
$_ll_command is no longer defined, so the test evaluates to false, failing to include the ls_with_file_shortcuts function.
@ndbroadbent ndbroadbent merged commit 905bd3d into scmbreeze:master Oct 22, 2012
@ndbroadbent
Copy link
Member

Thanks for this! That was very helpful, especially the idea to test the commands instead of the OS.
I've merged your pull request and have amended it a bit, as well as adding some more comprehensive tests for the ls_with_file_shortcuts function.
One note is that readlink does exist on OS X, but doesn't accept the -f flag, so I've changed the test to reflect that.
Thanks again!

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

Successfully merging this pull request may close these issues.

None yet

2 participants