Simplify entrypoint script a little bit

[d3adb5]

Refactor the entrypoint script to make it more readable for programmers.

Since variables should be documented in the README.md, comments were removed where I felt they were superfluous, and the ones that felt necessary to identify sections of the script were shortened, if possible.

As there was an instance of Bash's extended builtin test command ([[]]), all tests that were previously using the POSIX command were switched in favor of it. The shebang mentions Bash, after all.

Another style choice was inlining a few if statements where possible, and even using return instead of echo in array_contains. The functions were shortened, one even "flattened".

Bash's readonly and declare were introduced.

A couple of items from the ShellCheck output were addressed in 7f3c2c1. The relevant items are SC2129 and SC2145.

Instead of an overarching if statement in the while loop that processes config variables, grep was used in its input in 6b48b36.

Finally, trailing whitespace was removed, though this is frowned upon by many. Thought I would, since there were only three instances of it.

[Krzmbrzl]

I originally avoided grep as it returns a non-zero exit status if it doesn't match anything, which in combination with the -e flag would abort the script. To void that some special workarounds are usually necessary. I don't seem to find those in your version. Did you test that a non-zero exit code of grep never crashes the script?

[d3adb5]

The changes generally LGTM.

However, I am not particularly fond of removing comments. Yes, there exists the documentation in the README, but imo that should not replace in-source documentation in form of comments.

I left a few since I thought they were useful, but before the entrypoint seemed a little overcommented to me. If you still feel they should be present, I can push a commit restoring the previous ones, or drop ec402cb.

[azlux]

Hi, I like your commits. It's change a little bit our goal with your changes. We want, at first, a fully compatible POSIX script, but we face many difficulties with regex (workaround are horrible to read/code). So because we will not do it anymore, your PR is relevant, especially about readonly, declare , [[.

The only change I don't like is removing the comments. As simple dev on my free time, without comments, I need to check a lot the docs to read the script. Maybe making them less verbose (and thinner), I'm agree with that, but don't remove it for now.

[d3adb5]

I restored most comments and renamed a variable for good measure in 0f4fd08.

Since you said Bash is now the target, I went ahead and included readarray and even an associative array. I would've liked to keep the RegEx matching to just Bash, but thought it was trading readability for unnecessary performance. Instead, I'm using sed -E to retrieve exactly what we want from the bare-bones config file.

Sorry for adding even more code to be reviewed. Hope this isn't too tiresome for a first PR. :sweat_smile:

[Krzmbrzl]

Sorry for adding even more code to be reviewed. Hope this isn't too tiresome for a first PR. sweat_smile

No worries - I'll have another deep look at your changes sometime soon-ish :)

[Krzmbrzl]

Maybe we can increase the readability of the code by introducing a separate function normalize_config_name that does the upper-casing and underscore-removing And then use that in both places. That would facilitate potential future refactorings

[d3adb5]

I went one step further and removed MUMBLE_CONFIG_ from the beginning as well in the same function. Before I commit, what do you think?

normalize_name() {
    local uppercase="${1^^}"
    local no_var_prefix="${uppercase#MUMBLE_CONFIG_}"
    echo "${no_var_prefix//_/}"
}

The one problem this might cause is config options that contain some case of mumble_config_ in the beginning of their names. I thought, just like a potential collision of config options differing only in case and/or underscores, it was an easy assumption to make, but I want to know what you think.

With the above, we can just use ${option_for[$(normalize_name "$config")]} and ${option_for[$(normalize_name "$var")]}.

[Krzmbrzl]

I see how this would turn in handy and I certainly think that the assumption that no actual config name will start with mumbleconfig is more than justified. The only thing that itches me a bit about the proposed solution is that this seems to let the function do more than one specific task and we just rely on that particular task being a no-op for all use-cases other than when we actually want that to happen...

From a practical point of view, that'd work just fine, so this is a purely principal critique. However, given that this script is not expected to become part of a huge codebase, I think it'd be fine to use here.

So to sum it up: Personally, I wouldn't include this into that function, but I am not bothered too much by it either. So your call here. However, if you decide to use the function as proposed, I think it warrants a comment stating that we expect the prefix-removal to be a no-op for most cases in which this function is called. Just to make everyone aware of this assumption.

[d3adb5]

The only thing that itches me a bit about the proposed solution is that this seems to let the function do more than one specific task and we just rely on that particular task being a no-op for all use-cases other than when we actually want that to happen...

I can understand by principle it does exhibit multiple behaviors. Like you said, it isn't part of some huge codebase, and I would've left it as-is, but I think I managed to find a solution that is both brief and "principally moral" in cf6a9fb:

# Does one thing, doesn't care about prefixes.
normalize_name() {
    local uppercase="${1^^}"
    echo "${uppercase//_/}"
}

# ...

while read ...; do
# ...
done < <( printenv --null | sed -zn 's/^MUMBLE_CONFIG_//p )
# ^ Retrieves vars and already filters out the MUMBLE_CONFIG_ prefix

[Krzmbrzl]

Thank you for your contribution :+1: