search

sio / bash-complete-partial-path

Enhanced file path completion in bash (like in zsh)
Apache License 2.0
54 stars 2 forks source link

Add OSX support via gsed #1

Closed Unlearn closed 6 years ago

Unlearn commented 6 years ago

OSX's sed is BSD's, which causes compatibility issues. Attempt to use gsed instead installed via $ brew install gnu-sed

Unlearn commented 6 years ago

Ah. My spacing width is wrong. Feel free to reformat, or close.

sio commented 6 years ago

Thank you!

I didn't know about that OSX pecularity. From what I've read, converting existing regex to BSD sed would be rather hard and error prone. I think you've chosen the proper direction when decided to introduce sed's executable detection.

I have furthered your approach and published a modified version at branch macosx-sed. Could you please test it on OSX? My changes are documented in commit message for 3f60c8f

Unlearn commented 6 years ago

Hi @sio, I can't comment on the new branch directly, so I'll add comments here.

Your changes look good, everything works. Should be good to merge.

I considered a new function for the additional check also, however I always assumed variable assignment from echo's would break the variable in an error state. eg. _SED= would become "Completion script requires GNU sed, please install it with brew".

Is that what >&2 avoids? I would be interested in your take on the safety of variable assignment via echos in shell scripting.

On a side note, OSX is now macOS, if you want to correct such details for completeness.

sio commented 6 years ago

Is that what >&2 avoids?

Yes, that means "redirect echo output to stream no.2 (which is stderr)". When assigning _SED value, bash assigns only the value of stream no.1 (stdout), so the error message will never be assigned to the variable.

More information about standard streams: https://en.wikipedia.org/wiki/Standard_streams standard streams

sio commented 6 years ago

Thank you for testing the modified version. I will fix OSX naming and merge the changes later today

Unlearn commented 6 years ago

Thanks for the additional info. All makes sense. I've got some of my own scripts to refactor now ;)

Any additional testing on macOS I can do with future tweaks, PRs etc just let me know.

sio commented 6 years ago

I've just merged the macOS support into master branch, thank you for your contribution! I'm glad that you find this project useful and I hope you will continue using it.