As Requested: Some Comments & Thoughts (Part 1)

[ghost]

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!

https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L69-L100

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.conf

It'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.

https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L112-L128

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"

https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L142-L143

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...

https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L176-L182

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. :)

[CreativeCodeCat]

awesome ideas I will defo take your advice and switch some of the stuff that needs cleaning up I'm pretty new (4 months) at pure bash and use to use coreutils way too much for bash stuff

[CreativeCodeCat]

@terminalforlife i did try this and it came back with nothing? is there something im doing wrong? it seems to work in a script on its own but not inside the bash script itself

usage() {
	read -d '' <<-EOF
	Github Project Manager v$VERSION
	manage, switch between templated licenses, and set languages

	$AUTHOR_NAME		  -- '$AUTHOR_NAME'
	$AUTHOR_EMAIL		  -- '$AUTHOR_EMAIL'
	$AUTHOR_WEBSITE		  -- '$AUTHOR_WEBSITE'
	$AUTHOR_GIT			  -- '$AUTHOR_GIT'
	$LICENSE_DIR		  -- '$LICENSE_DIR'
	$LANGUAGE_DIR		  -- '$LANGUAGE_DIR'

	[ENV=VAL ...] $(basename "$0") <flag>

		-initialize	   -i @project_name			initialize a git repo for a project
		-list-licenses	 -l						list available licenses
		-list-languages  -ll					list available languages
		-current-license -c						show current active project license
		-pick-license	 -p						pick a license in a FZF menu
		-pick-language	 -pl					pick a language in a FZF menu
		-preview-license -P @license_name		preview a license template
		-remove-license  -r						remove all licenses from the current project
		-search-license  -s '<query | patten>'	search for license
		-template-help	 -T						print help for templating
		-new-template	 -t						create a new license template
		-delete-license  -R @license_name ...	delete a license from templates
		-new-config	   -C						create a default config overwriting current one
		-check-conflict  -k						check if current licene(s) are conflicting

		-help			   -h					show this helpful list of commands

	PROCTL_COLOURS=0|[1]						turn colour on (1) or off (0)			[${PROCTL_COLOURS:-1}]
	EOF

	printf '%s' "$REPLY"
}

edit removing set -e seems to have fixed this issue

[ghost]

Oh, glad it's sorted now. Personally, I don't recommend using set -e and various other similar debugging flags, unless you're actually debugging, otherwise it can lead to issues like this one, a lack of understanding of what's happening, and I often find people who always leave debugging flags aren't very good at error handling. It's up to you where you stand on this. Perhaps I'm just so used to handling empty variables myself, that it's a non-issue for me — IDK.

There are similar arguments in PERL land too, where some will recommend against use autodie; because it leads to a lack of understanding (I got caught out by this!) and proper error handling. I think the same applies in BASH. However, I would always recommend use warnings; and use strict;, so perhaps I'm not 100% sure on this mindset.

BTW, that usage output is far too wide, requiring a rather large terminal to view the output properly. Try to keep all standard and error output given to the user within 80 columns. You want things to be presentable and easy to read for as many users as possible, after all.

[CreativeCodeCat]

i really dont understand read -p ' Do you want to reinitialize this git repo? [y/N] ' how do i get the output of that without the print?

would it be like

read -p '   Do you want to reinitialize this git repo? [y/N] ' yn
[[ ! "$yn" = "y" ]] && exit   

[ghost]

Ah, so, the read builtin, in this case, is both printing the prompt (hence the -p flag) and storing the response in $REPLY (default in BASH), if no identifier (variable name) is provided. If you want to print the result the user entered, then of course you'll need to echo or printf or something to display it.

[[ ! "$yn" = "y" ]] && exit

Yes, except I would enter:

[[ $REPLY == y ]] || exit

The == is just a more modern, BASH way to say 'equal to' (strings, if using [[ keyword). The quotes aren't needed, because at the right of == is a glob pattern, and I just don't personally see the need to quote that, since BASH needs to interpret the glob syntax anyway, same with the REGEX syntax when using =~. I used $REPLY, because that's the default with read in BASH if no identifier is provided.

Please, please avoid double-negatives in your logic! It's a huge no-no, because it unnecessarily confuses the reader. Think of it this way: your initial approach was saying "if it's true that it is not true" when you could instead just say "if it's not true". Actually, technically speaking, the way you phrased it was "if it's true that it's not true that it's true", because of where you placed the negation. Lol Sometimes a double-negative is unavoidable, but most of the time I find it's easily avoidable.

BTW, regarding exit: do note that if no integer is provided for the exit status, then the exit status of the previously executed command is used, as explained in help exit. Something worth keeping in mind. This is part of the reason why I'm explicit with those integers.

[CreativeCodeCat]

cheers got it working well with most of these improvments :)

[CreativeCodeCat]

yet so clean in vim its doing my head in

[ghost]

That's because of the tabs. Use whitespaces within the actual heredoc, except of course any leading tabs which - in <<-EOF makes BASH ignore.

[CreativeCodeCat]

besides the mess i think this one if pretty much addressed with my latest push :)