search

paulirish / git-open

Type `git open` to open the GitHub page or website for a repository in your browser.
MIT License
3.29k stars 246 forks source link

Replace dirname with zsh expansion modifier #171

Closed ratijas closed 1 year ago

ratijas commented 3 years ago

Basically, these are modifiers to parameter expansion. The parameter here is $0, the :A modifier converts it into an absolute path resolving symbolic links in the process, the :h modifier takes 'head' by stripping 'tail', i.e. removes last path component.

Read more about built-in zsh expansions and substitutions at zsh documentation website: http://zsh.sourceforge.net/Doc/Release/Expansion.html#Parameter-Expansion

ratijas commented 3 years ago

@paulirish Could you, please, take a brief look at this, sir?

ratijas commented 3 years ago

@derimagia Maybe you could review it?

paulirish commented 3 years ago

lol definitely more cryptic. That alone is enough for me to not want to land this. Does this fix anything?

The whitespace fix is good, obv.

ratijas commented 3 years ago

Hi @paulirish,

It's good to hear from you!

Well, it does not "fix" anything per se (expect that process substitution might be a subject to parameter expansion under certain circumstances, but that wasn't addressed anyway). But it gets rid of an external command invocation (and a subshell), so it's definitely an improvement in terms of speed.

And by the way ${0:A:h} seems to be a pretty well known idiom in bash/zsh, just like %~dp0 in cmd.exe, so after all it's only cryptic for those who are not into shell scripting. I mean, if you are someone who already know about process substitution $(syntax), than you're only one step away from parameter expansion modifiers.

I could add comment to the code explaining the concept, but I don't think it worth it — even DuckDuckGo and Google can tell you that.

ratijas commented 3 years ago

I think I perfectly defended my positions and meaningful conciseness of this PR. It's your turn to tell what do you think about it, @paulirish.

paulirish commented 1 year ago

(late reply but…)

@ratijas i appreciate your passion here and also your time to justify the PR.

However I just can't accept that swapping $(dirname $0) for ${0:A:h} is an improvement. This operation isn't too sensitive to perf, so that angle isn't as compelling.

I pretty much agree with how this person put it: https://stackoverflow.com/a/22402017/89484

Cheers and sorry.

ratijas commented 1 year ago

ok ¯\_(ツ)_/¯