Skip to content
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

Bash constants readonly #18

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Bash constants readonly #18

wants to merge 2 commits into from

Conversation

Jontified
Copy link
Contributor

@Jontified Jontified commented Jun 28, 2022

In bash guidelines add that constants should be readonly always and that non-constants that are not modified after declaration should be readonly on a best-effort basis.
Remove the part of the guideline that says that COMPILER_FLAGS is an example of a constant that can be modified after declaration. After discussion this was concluded not to be the case and that this variable should not be screaming snake case.
Update example to reflect this.


This change is Reviewable

Clarify that we should use readonly when a variable is constant as well
as on a best-effort basis when the variable is not constant but should
not be modified after declaration.
We no longer consider this to be a good example of a constant variable.
Update example to reflect this.
@Jontified Jontified requested a review from faern June 28, 2022 14:50
Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Jontified)


bash.md line 72 at r1 (raw file):

functions are not constants.

Constant variables should always have `readonly` applied to them. Non-constant variables can

Maybe give this its own heading, like ### readonly or something? I feel like it does not fully belong to the above header. 🤔


bash.md line 215 at r1 (raw file):

set -eu

SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

We have to discuss and figure out if we want to treat this as a constant or not. And should it have readonly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants