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.

Sign up for GitHub

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

Jump to bottom

Gracefully handle unbound parameters #1903

Merged
merged 12 commits into from
Aug 31, 2021
Merged

Gracefully handle unbound parameters #1903

merged 12 commits into from
Aug 31, 2021

Conversation

gaelicWizard
Copy link
Contributor

@gaelicWizard gaelicWizard commented Jul 26, 2021 •
edited
Loading

Description

Expressly handle parameter values as empty when the parameter is not set at all, instead of letting comparisons fail for missing arguments. Change $BASH_PREVIEW to "${BASH_PREVIEW:-}" and similar for $BASH_IT_LOG_PREFIX, $BASH_IT_LOG_LEVEL, &c. This helps towards #1696.

Motivation and Context

This reduces errors by explicitly testing for blank values rather than silent syntax errors. I'm very careful about what code I allow to run, and I use set -u to ensure that I'm not inadvertently confusing an edge-case when scripting. There's a lot of unbound variables here.

How Has This Been Tested?

This is my live branch in use on my own system now.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

NoahGorny
NoahGorny requested changes Aug 3, 2021
View reviewed changes
Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

Small nit that the tests picked up- great idea and great work @gaelicWizard !

lib/log.bash Outdated Show resolved Hide resolved
@gaelicWizard
Copy link
Contributor Author

gaelicWizard commented Aug 9, 2021 •
edited
Loading

I've rebased on current master branch. Anything else needed for this PR? 😃

NoahGorny
NoahGorny approved these changes Aug 11, 2021
View reviewed changes
Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

nice job!
I have a question though- how can we prevent new PRs from adding new unbound parameters?
we should have a test that will fail this kind of thing- what do you think @gaelicWizard ?

@gaelicWizard
Copy link
Contributor Author

we should have a test that will fail this kind of thing

I found a StackOverflow q/a which includes an example of an unbound parameter failing to fail a [ -n test which freaked me out a while back: if the parameter was empty, the test would fail; if the parameter was not empty, the test would pass; if the parameter was unset, the test would pass. But as far as just detecting it, setting shopt -so nounset (aka set -u) somewhere in BATS should make it easy to spot. I'm not familiar enough with BATS to offer a patch for that right now, but I'm learning BATS so maybe next week.

@NoahGorny
Copy link
Member

we should have a test that will fail this kind of thing

I found a StackOverflow q/a which includes an example of an unbound parameter failing to fail a [ -n test which freaked me out a while back: if the parameter was empty, the test would fail; if the parameter was not empty, the test would pass; if the parameter was unset, the test would pass. But as far as just detecting it, setting shopt -so nounset (aka set -u) somewhere in BATS should make it easy to spot. I'm not familiar enough with BATS to offer a patch for that right now, but I'm learning BATS so maybe next week.

If you add set -u right at the end of the setup function in test_helper.bash, it will generate the wanted behaviour. I noted that we have some vendored code with unbound variables, so we can also try to add PRs upstream to fix it.

@gaelicWizard
Copy link
Contributor Author

I've rebased on current master, but I haven't had time to do BATS yet. 😃

@NoahGorny
Copy link
Member

I think that we should just open an issue about it. No need to clutter this PR with even more changes.

@NoahGorny NoahGorny mentioned this pull request Aug 26, 2021
Closed
@gaelicWizard
lib/log: handle undefined variables
02c13b7 
If the user hasn't defined BASH_IT_LOG_LEVEL, then the integer comparison fails. Handle it by defaulting to '1'.
If lib/log is loaded improperly, the BASH_IT_LOG_PREFIX may be undefined. Unlikely, but no harm in handling it too.
Likewise, if no theme is loaded, then $echo_green, $echo_normal, et al are not defined.
@gaelicWizard
lib/preview: handle unbound parameter
bcd67db 
If $BASH_PREVIEW is unset, treat it as blank.
@gaelicWizard
Deal with unabound BASH_IT_RELOAD_LEGACY
5001995
@gaelicWizard
Handle unbound variables
f71fa5b 
Handle BASH_IT, BASH_IT_OLD_BASH_SETUP, BASH_IT_THEME, BASH_THEME, and PROMPT.
@gaelicWizard
lib/log: handle undefined variable
233fcc3 
Expressly deal with if $echo_yellow hasn’t been defined
@gaelicWizard
lib/utilities: handle unbound parameter
0eff449 
Expressly handle unbound $BASH_IT_GREP when testing for value
@gaelicWizard
lib/helpers: handle unbound parameter
8fb75cf 
Expressly handle $BASH_IT_AUTOMATIC_RELOAD_AFTER_CONFIG_CHANGE being not-set as being blank.
@gaelicWizard
lib/helpers: handle unset parameter
bd9f401 
Expressly handle $BASH_IT_REMOTE as blank when variable is not set.
@gaelicWizard
plugins/base: unbound BASH_IT_LEGACY_PASS
cd65e09 
Expressly handle as blank when $BASH_IT_LEGACY_PASS is not set.
@gaelicWizard
completion/system: give up hope
0c41244 
Give up and accept defeat that bash-completion can't reasonably be audited for unbound parameters. Wrap invocation with disabling strictness, and restore after if it was enabled.
@gaelicWizard
Tests for 'default' prefix
3654198
@gaelicWizard
lib/log: default to no logging at all
8540874 
Set the default when BASH_IT_LOG_LEVEL is unbound to log level none: no warnings or errors are reported at all.
@NoahGorny NoahGorny merged commit 5782325 into Bash-it:master Aug 31, 2021
@gaelicWizard gaelicWizard deleted the unbound branch September 5, 2021 23:01
@gaelicWizard
Copy link
Contributor Author

It has come to my attention that this entire branch and my work would have been wholly unneeded if only the .shellcheckrc did not have the SC2154 test EXPLICITLY DISABLED.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NoahGorny NoahGorny NoahGorny approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gaelicWizard @NoahGorny