-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
aliasrc fixes #1459
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
aliasrc fixes #1459
Conversation
They are |
The original change belongs to me and this is so unnecessary. Shellcheck won't work with Zsh by the way. The shebang should have been @RetroViking Even if you have directories, the script would work with them. I sent you a PR with the fix. Here it is for reference:
This is more aligned with the Zsh syntax. |
due to zshisms in se()
this happens every time a script is selected and another one matches before it, this happens if you select a script such as "sd" while having another one called "cron/sdo"
Nice catch! I've updated the shebang
Almost, I've changed it to
Never said the contrary
It's nice to have the shebang match the actual format, just to avoid throwing off users
I know, this is just to avoid misleading shellcheck results.
Answered on the PR itself |
You mention some problems like:
But I can't replicate this on my machine. I created By the way, if you want to keep the extension you just need to delete the Funny how something as simple as a script-selector can be implemented in a quintillion possible ways. Just out of curiosity, I actually benchmarked 5 versions of
Some thoughts on this:
Bottomline: these diferences in speed are so small no human mortal will probably ever notice them, so it ultimately comes down to aesthetics and style. emrakyz's Classic LARBS is also cool because The scripts I used, for others to benchmark.How to test this:
This will run each script 1000 times and show you the "wall clock" time it took to run it under the If you want to actually use one of these scripts for your Classic LARBS:
Current version (emrakyz)
Current version, but improved by emrakyz for correctness:
aartoni (present PR)
Pure POSIX shell (no external programs other than the obvious). The
|
Interesting enthusiasm. I have benchmarked many shells including Dash, Bash, Zsh, Ksh, NuShell, Fish and some other niche ones. Zsh has the most built-in features while being the fastest (excluding some niche cases where Ksh is miles faster); especially if you use Clang, Ofast, ThinLTO, PGO, and build statically. Zsh has many internal optimizations around variable, array collection and slicing. A script with Zsh-isms will almost always work faster than other types of scripts; especially when you otherwise use external commands (if the data is extremely big, external programs can be faster than built-ins though, for some rare cases). Keep in mind that, speed comparisons need to match with features. My script removes path, extensions from the files, and matches the file all in the same pass without even using a single command. It also handles all types of filenames. It won't split incorrectly. On the other hand, Zsh's slicing here is miles better than find's output. It's safer and more correct. Zsh is specifically optimized for file handling (especially with weird filenames) compared to other shells. Also, with My version is simply less complex and completely minimal, and built-in. My recommendation is: DO NOT use same names for different scripts. In a Linux path, you shouldn't have two separate binaries with same names anyways. It doesn't matter where it is located. A binary should have a completely unique name no matter where it is. With this way, you don't even need the improved version. And no, there is no way for the pure shell version. That's why I specifically used Zsh syntax for the PR. Your current version is also wrong, I don't use
This is the original implementation. This implementation can select: For my own system, I don't even use this implementation (I use even a simpler one) because I name all my scripts with So a better approach is simply better. You don't even need the upgraded version. This is just safe, accurate, native, all built-in, minimal, single pass method. There is no need to overcomplicate. Sometimes, instead of creating fixes for edge-cases, you need to change your methods handling things. A simple example: Instead of handling filenames with spaces, special characters etc; simply do not name files with these characters. I have been using the same exact approach for a very long time for both my script and my configuration files with nested directories in |
Yeah, I know. I mentioned why I use If you read the comment before the latest changes, kindly re-check it because there's some new important info. |
@RetroViking You can use Zsh built-in randomizer for that. An example (this can be used for wallpaper randomization, for example):
Zsh also has features for Example:
Zsh can even be replaced with
This above loads the file, extracts segments, uses different lines and segments from the file's text. It then uses those for calculation. All without using |
For example, I use something like this for my statusbar dash script:
When I was building the script, I naively tested this command with I recall you're a Wayland user, which is more minimal and "the future" in a sense, so maybe that's why Myself, I'm more of a "classic car" kind of guy that's interested in the old Bourne shell and UNIX days. In my head, even |
I agree with that, @emrakyz version does more string manipulation while mine does some implicit unsets and shadowing and edit checks on the I was thinking that we likely want to use
Good recommendation for a general use case, however there are applications that have architecture-specific binaries and rely on a
I completely agree with you, at least within I am now realizing that mine may be a nicher use case than I figured it to be, so I will likely wait a bit and decide whether to change this to a slimmer PR or go through with this version or @emrakyz' version from my branch. |
True. If you set a I'm curious, does the current version work for you? It works flawlessly for me, but in the past there was an oddity where By the way, no need to stress about the shebang. It makes no difference when sourcing scripts.
|
These kinds of edge cases are pointless to trace. Edge-case tracing makes things extremely complex. You know, and you control your system. Create aliases, shortcuts knowingly. By the way, calling The script with its current form is very good already for Luke's setup. |
@emrakyz, is it different for you?
It's up to everybody to decide when the system must be changed, and when one's own habits must be changed. For example, in my case I decided that doing some small tweaks to my scripts, to allow for safe handling of filenames with spaces, is less cumbersome than ensuring that all my filenames are "correct". Set up your scripts once, never think about it again. For |
I guess I use simple |
@emrakyz and @aartoni , did you know that Try this:
emrakyz, what is your Edit: about In short, shellcheck can be deceiving if you don't know exactly what you're sending it to check. Also, Since my scripts don't have extensions, I'm considering using |
Actually I use the Rust-based You can even write the directory names wrong, and still go there. Let's say there is a folder like this: You can even go there with: It also offers options to show you most visited directories in This can also be used in TUI file managers. I use the Rust-based On the other hand, I also have a config in my
If you want to use variables instead, then you should either use eval, or two pass methods, but I don't use variables. |
I know, I know, this is just a very small change that has already be addressed. For the reason why I wanted to change it, consider this hypothetical scenario: you have a Python script with a bash shebang, that would make no sense, you would change it as soon as you notice it. So this case is even worse because a Linux/LARBS noob may not get the subtle information that it uses a different language (i.e. Of course, some experienced user like us in this thread would get that.
You're right.
I didn't know! I think this might be both the best and fastest way of using it. I will update the PR in a few days!
Well, your script had a bug and I have fixed it. By talking about it @RetroViking found a way of fixing it by using a built-in feature of Overall I am convinced that opening this bug fixing PR has been useful. |
@emrakyz
They are useful IMHO. With your bookmarked directories and the vanilla LARBS
You can also use Thanks for sharing your
You can use
It is always useful, even if the PR is initially not perfect or a bit confused, because further discussion can lead to discoveries that would have never happened otherwise. Without this PR I wouldn't have learned the things I did, from reading the comments and thinking about this, so I've certainly been enritched by it. |
I don't think the fix approach is good though. Fixing something by changing the approach or losing functionality is not the best way. I think it must have been an issue instead of a pull request. PR without building a good solution, covering all the previous functionality is not the most optimal approach: First of all:
The only problem can be accessing two different If you want to also keep the variables intact, then:
This solves the sub-directory, same name script problem by also removing path, extension keeping just the relative path for sub-directories. Clean and concise. |
Sorry for the negligent merge on my part. My idea for this So it should remain |
/bin/zsh
sincese()
uses zshisms (known since last rework)se
c
({${(M)s:#*/${c}*}
), leading to incorrect matches. For example, if you have a scriptcron/sdo
and choosesd
infzf
you'll editsdo
~sc/
from the full path to display a partial path and keep the extensionsI've provided a less verbose version of this explanation in the commit messages as well.
Edit: bash -> zsh