-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
general: Remove redundant usage of cat
#1901
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine if cat
is truly redundant here, but I'm too ignorant here to feel comfortable approving... would like the other maintainers to take a look.
@belak any view? |
I held off on this one before because I wanted to test it myself - it might be a little while until I can do that |
This is an interesting PR, to be sure - I didn't know this was possible. It mostly makes sense, but it's not the direction my brain would go for these. I like the change in modules/python/init.zsh - that's a fairly common convention when reading from files which I've seen in multiple places before. I have no strong opinions on the heredocs - they work, as far as I can tell, they're solid, and will probably perform marginally better. However, if it was purely my decision, I probably wouldn't push for that replacement - it's a bit less readable and doesn't follow with common conventions (I've never seen that before - really neat that it works), especially as seen in other shells. That hasn't stopped us before, at least when there's a zsh convenience, but for me in this specific case it wouldn't be worth it. I'm a fan of the replacement in modules/python/init.zsh, and if you feel strongly about the heredoc change or think it adds enough value, you're definitely welcome to merge it as-is. |
Yes, readability does take a hit, I admit. But Zsh "hieroglyphs" (with all esoteric single-character flags and switches) probably don't focus on readability anyway I guess :) One probable reason we don't see this convention around is probably because of POSIX compliance. Besides, pure Zsh optimized interactive shell is a niche space. I'll hold off merging this just yet. Let's see if this PR can evolve a bit. |
In general, the substitution ‘$(cat foo)’ may be replaced by the equivalent but faster ‘$(<foo)’. See: https://zsh.sourceforge.io/Doc/Release/Expansion.html#Command-Substitution
In general, the substitution ‘$(cat foo)’ may be replaced by the
equivalent but faster ‘$(<foo)’.