As Requested: Some Comments & Thoughts (Part 1) #1
Description
Hope this is what you had in mind. :) I'll comment at the top of each snippet. You might want to reference this issue in any commits which might address this issue.
Is there a specific reason for this line? Because if $HOME is empty, then you won't get anything from ~ anyway, so I'm 99.9% sure this is a redundant line. I tested this just now, with HOME= followed by echo ~ and got nothing, as expected.
https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L25
I recommend using the type builtin over the command builtin for dependency checking, because type -P will only look in $PATH for executables, whereas the old-school approach you used can pick up other things, like functions. Unless you're reckless with your names, this probably won't be an issue, but I do think it's probably one of those 'good practice' situations.
https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L27
I strongly recommend against using aliases in a script, despite how clean it can look. Unless it's your BASH configuration, of course. You cannot rely on the user having aliases available in their BASH installation, and it would likely lead to confusion on the reader's part, because it's surely unexpected in a program/script.
I do appreciate you thinking out of the box though, and it certainly does look concise.
https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L43
You wish to escape the escapes? Parameter expansion's pattern substitution would've sufficed here, which would also be much more efficient. IE:
escape() { printf '%s\n' "${1//\\/\\\\}"; }https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L61
Personally, and I suppose 'in my opinion', I find that if I'm needing to use -maxdepth N in a script with find(1), then I shouldn't be using find(1) for that task, unless it's just a quick-'n'-dirty thing. This is because you're loading up the find(1) program unnecessarily. Here's what you could do with the shell itself, with minimal effort:
for File in "$LICENSE_DIR"/*; {
if [[ -f $File ]]; then
printf " $BRIGHT*$RESET @%s\n" "$File"
fi
}What about -L? You might ask. Well, I don't understand why following links even makes sense in this case, because you're not working recursively. Have I missed something? If not, then I think this function would be better.
If you need to include hidden files, use {.*,*}.
Same goes for both functions, but TBH, if I were writing this, I wouldn't have written two functions which do essentially the same thing — I'd have written one function which could do both, by using a flag of some sort; it would've saved repetition.
This possibly would've been a good opportunity to use indirect expansion using a name reference (nameref).
https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L65-L67
This could've been much easier with a heredoc; it would've saved you a lot of hassle, then and in the future.
If only you used tabs:
usage() {
read -d '' <<-EOF
Some lines go here.
Some more lines go here. Tabs are ignored.
Imagine the indentations were tabs.
It's even a teeny-weeny bit more efficient.
EOF
printf '%s' "$REPLY"
}So much cleaner and so much easier to work with!
Again, this could've been so much nicer, cleaner, easier to work with, and, much like the above usage(), even a bit more efficient, if you used tabs:
read -d '' <<-EOF
export AUTHOR_NAME='$(git config user.name || echo "${USER}")'
export AUTHOR_EMAIL='$(git config user.email || echo "${USER}@gmail.com")'
export AUTHOR_WEBSITE='https://example.com/'
export AUTHOR_GIT='http://github.com/$(git config user.name || echo "${USER}")'
export LICENSE_DIR='$LICENSE_DIR'
EOF
echo "$REPLY" >"$HOME"/.config/license.confIt's things like this which have me wondering why people poop all over tabs, in programming. If you're a Vim user, you can :set tabstop=2 and you're good to go. :P
BTW, I recommend using variables for paths more often, at the very least, if you plan to reuse them. Like in this case, I found a few instances where that same path was used. It's just what I'd consider 'good practice' because it avoids (potentially disastrous) bugs as a result of typos and it makes it easier to change these things in the future.
Instead of separately printing and then reading from STDIN, you can combine the two, which is also more ever so slightly (lol) more efficient — you're in BASH, after all:
read -p ' Do you want to reinitialize this git repo? [y/N] 'You probably also don't want to use the -r flag all the time, despite how people will often tell you to, unless of course you're planning to parse escape sequences provided by the user. While it's probably benign here, it's a flag you will likely rarely actually need, so that'll save you a couple of key-strokes.
As I say: "use what you need"
Quite a bit of an optimization, and to make it much clearer:
check_valid_license() {
lic=$(echo "$1" | sed "$LICENSE_SED")
if [[ $1 != \@* ]]; then
error "License '$lic' is not valid"
exit 9
elif ! [[ -f $LICENSE_DIR/$lic ]]; then
error "License '$lic' does not exist"
exit 10
fi
}Why?
- grep(1) wasn't needed
- REGEX wasn't needed
- The
exit-s weren't as clear as they probably should've been - You had a confusing double-negative in your test(s)
- The logic wasn't very clear or concise
- You used 2 subshells for no reason
Actually, regarding the subshells, I believe you exited from those subshells, meaning you weren't exiting from the program itself, which is presumably what you wanted, so that was a probably a bug.
It's at this point I'd suggest something to replace sed(1), because I suspect that's not needed, but...
It's getting super late and there is a lot to get through here, so I'll try to resume this tomorrow, if I get the chance. I hope this stuff was helpful! Maybe it'd be better to do this in chunks. Let me know what you think. If you disagree with anything, that's okay. :)