use UPCASE in global variable names / shellcheck partial snippets

[meleu]

Request

Declare all global variables using UPPERCASE (e.g.: $ARGS, $DEPS).

I understand that this can be an issue for backward compatibility (bashly users who are already referencing $args in their scripts). So an alternative solution would be to provide a flag like upcase_global_variables in bashly-settings.yml .

UPDATE: this was solved by adding a configuration option via this PR

---

Motivation

Using UPCASE in global variable names is pretty common. But to be honest, the reason I'm opening this discussion because shellcheck complains when we refer a variable that is not (visibly) assigned.

Example: when I try to use ${args[something]} shellcheck complains that args is referenced but not assigned [SC2154].

Using UPCASE in the variable name solves this issue. As explained in the SC2154 page

Note: This message only triggers for variables with lowercase characters in their name (foo and kFOO but not FOO) due to the standard convention of using lowercase variable names for unexported, local variables.

I can solve it in my end by using # shellcheck disable=2154 right above the lines where I use references to $args.

Alternatively I could disable this shellcheck in the whole file or in the whole project, which is something I don't recommend, since this is an important thing to have enabled (to check that the variable is indeed assigned, and that the name is not misspelled).

Using shellcheck as a shell script linter is (hopefully) popular these days, then it would be neat to be able to use the power of bashly with no need to tweak shellcheck configs.

[DannyBen]

Well - you mention two separate issues:

Shellcheck

In regards to shellcheck - you should not shellcheck the partial snippets, but just the final script.
All the code generated by bashly itself is both shellcheck and shfmt compliant. This is also ensured during CI.

So if you have a shellcheck violation, it is either because you are checking a partial script or your own code is indeed not compliant.

More info in Bashly Homepage > Testing Your Script.

Uppercase globals

In regards to upper case globals - this is unlikely to happen.

Bashly is opinionated. It is my view that CAPITAL_VARS represent variables that came from the outside (environment).
Like in almost all other programming languages, variables are lowercase. Bashly aims to make bash programming somewhat resemble programming in other languages, while staying true to the bash roots.

Although I agree that in some circles the standard could be CAPS == Global, I am not sharing that view and do not want to have capital letter variables plastered all over my script.

As always, I am happy to be convinced otherwise and I might change my mind.

[meleu]

Alright, let's separate the issues (I renamed the discussion title).

I think there's no need to keep discussing the uppercase globals thing. Let people decide on their own.

My main concern is actually about shellcheck (as an outstandingly useful tool that saves a lot of troubles).

Integrating linters in the code editor is a great feature that accelerates productivity, by decreasing the context switching (showing issues in your code as you type it).

I'm willing to write a bashly tutorial and encourage my coworkers to use it, but now I'm surprised you mentioned that we "should not shellcheck the partial snippets".

Maybe we should start another discussion thread to talk about this, but first let me ask:

1. Consider a public bashly tutorial (imagine even something to be in the official website). Do you think that enabling shellcheck on the editor while producing code to be used with bashly should not be encoraged?
2. Are you aware of other "spurious" shellcheck warnings that could appear in the partial snippets? (if the list is short, I can try to keep a "Shellcheck Caveats" session)

[DannyBen]

Well - to answer both of your questions:

I was never a fan of IDEs, so I do not know how shellcheck or other IDE integrations work on partial scripts.
I can guess however, that since the code is "partial" by design, that unless the IDE has a way of virtually placing that partial in its full context, it might fail in more than one places.

[meleu]

@DannyBen

do you think providing a flag like upcase_global_variables in the bashly-settings.yml is an acceptable feature?

[DannyBen]

do you think providing a flag like upcase_global_variables in the bashly-settings.yml is an acceptable feature?

At this point, I am leaning towards a medium no. The reasons being:

1. As far as I can tell, the way I interpreted it is the standard accepted way. Meaning: CAPITAL is for constants (readonly) and for environment variables (either incoming, or exported).
2. All these "internal" arrays are plastered all over the templates. Having these names depend on a configuration means a lot of places that will become less readable to maintain, and the templates are the most hard to maintain part of the codebase already.

I think there might be a way for you to clone the lowercase variables (ARGS=("$args")) using a before hook. Not sure about it, and not sure it is helpful - but thought I would mention it here.

[meleu]

Here's a (hacky) trick I used to solve this with no need to change anything in bashly code:

src/initialize.sh (also works in before.sh):

# shellcheck disable=2034
declare -gn ARGS=args DEPS=deps

Now ARGS is a nameref to the args, which means that both are references to the same content.

Same thing for DEPS/deps.

[DannyBen]

Oh, this is cool. I was unaware of the -n flag. Seems like a reasonable solution, and not too hacky.

In fact - if this works properly - this is something I can add as a configurable option.
Still not sure it is needed, but my second argument above about keeping a maintainable code, is gone with this.

Keep in mind that if you are going to use Catch All, or environment variables, there are also these vars: other_args, env_var_names.

[meleu]

I've just thought in a possible problem to adding it as a configurable option:

Confused users saying that they enabled the upcase_global_variables and are using ARGS for usual argument parsing but willing to use args elsewhere.

Maybe just add the declare -n thing and mention in the docs that both UPPER and lower are fine (another benefit: no configurable option means less logic to implement/maintain).

To be honest, I'm happy with my solution and it's a minor thing compared to all the benefits brought by bashly. I already feel grateful for what you've done! ❤️

---

I confirm the declare -n works properly, and I show a couple of examples below:

$ array1=(one two three)

$ declare -n NEW_ARRAY_NAME=array1

$ NEW_ARRAY_NAME+=(four)

$ echo "${NEW_ARRAY_NAME[@]}"
one two three four

$ echo "${array1[@]}"
one two three four

It also works fine if the declare -n is used even before the "namereferenced" variable exists (which is what happens when we use my trick in the initialize.sh).

Evidence:

$ cat nameref-poc.sh
#!/usr/bin/env bash

initialize() {
  # this is called before "$args" exists
  declare -g -n ARGS=args
}

run() {
  declare -g -A args=()
}

initialize
run "$@"

# --- let's manipulate them --- #

args+=([a]=1 [b]=2)
ARGS+=([c]=3 [d]=4)

echo "args: ${args[@]}"
echo "ARGS: ${ARGS[@]}"

# end of namedref-poc.sh
########################

$ ./nameref-poc.sh
args: 4 3 2 1
ARGS: 4 3 2 1

[DannyBen]

I didn't get this sentence:

Confused users saying that they enabled the upcase_global_variables and are using ARGS for usual argument parsing but willing to use args elsewhere.

But other than that, I think I can make the declare -n thing appear if a new setting is set.

[DannyBen]

Opened issue #586

The one thing I dislike about it is that if people use ARGS, they might think that they can use args for their internal means.

Also - I was thinking that if we do this, perhaps go one extra step:
Instead of letting users choose CAPS or no caps, maybe they can choose the var names:

args_varname=ARGS
deps_varname=DEPENDENCIES
other_args_varname=EXTRAS

Thoughts?

[meleu]

Solved via this PR