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

[WIP] Adds partial support for the unofficial bash strict mode #230

Closed
wants to merge 20 commits into from

Conversation

nkakouros
Copy link

@nkakouros nkakouros commented Dec 12, 2017

cc: @mbland

I run my scripts with set -euo pipefail at the very top. I run into some unbound variables when doing so. Maybe I'll run into more as I move into using this framework. I think it is good to have default values/explicit null to make assumptions in the code clearer.

On the part where I changed := to :-, I did so to avoid running the same value assignment twice.

This PR is regardless of #220, but it could prove handy if #220 starts getting implemented. This PR is meant to support those users that do want the strict mode in their scripts.

@nkakouros nkakouros requested a review from mbland as a code owner December 12, 2017 15:01
@nkakouros nkakouros changed the title Handles unbound variables in go-core Adds partial support for the unofficial bash strict mode Dec 12, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-21.8%) to 73.216% when pulling 2f126ca on tterranigma:unbound into b8d7140 on mbland:master.

@@ -26,7 +26,7 @@ _@go.set_search_paths() {
# A plugin's own local plugin paths will appear before inherited ones. If
# there is a version incompatibility issue with other installed plugins, this
# allows a plugin's preferred version to take precedence.
@go.search_plugins '_@go.set_search_paths_add_plugin_paths'
@go.search_plugins '_@go.set_search_paths_add_plugin_paths' || :
Copy link
Author

Choose a reason for hiding this comment

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

@go.search_plugins returns 1 some times. Based on the fact that without strict mode, the script executes without errors, I guess that when it returns 1, it is not an actual error. It is probably meant to help when @go.search_plugin is used in an if statement.

If the above is correct, @go.seach_plugin is used in an if statement only once in the whole project, while it is used 8 times as here. Should the function be redesigned or, if the above 'fix' is sufficient, should the same 'fix' be applied in all other 7 case?

@coveralls
Copy link

Coverage Status

Coverage decreased (-21.9%) to 73.14% when pulling cca38a8 on tterranigma:unbound into b8d7140 on mbland:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-22.7%) to 72.306% when pulling c1423d7 on tterranigma:unbound into b8d7140 on mbland:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-22.7%) to 72.306% when pulling 6bbf0c9 on tterranigma:unbound into b8d7140 on mbland:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-21.9%) to 73.152% when pulling 8201541 on tterranigma:unbound into b8d7140 on mbland:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-22.4%) to 72.673% when pulling 7e92dd5 on tterranigma:unbound into b8d7140 on mbland:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-22.4%) to 72.663% when pulling d8f6d9e on tterranigma:unbound into b8d7140 on mbland:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-22.4%) to 72.663% when pulling d8f6d9e on tterranigma:unbound into b8d7140 on mbland:master.

@nkakouros
Copy link
Author

nkakouros commented Jan 16, 2018

I have a question. When using bash autocomplete, there are around 4,5 commands that return '1' if no candidate for completion is found. When setting set -e, this stops execution at the first such command. This is no problem, as the completion would anyway return nothing in the command line. But when using trap custom_function ERR without set -e, the commands that return 1 cause custom_function to run as many times as there are such commands. This is bad as the user receives unexpected output when instead nothing should be printed in the command line pressing TAB. Should this PR try to 'fix' these commands as well? In my own projects, I take care of the situation in my custom_function by searching for libexec/complete in "${BASH_SOURCE[@]}" and doing set -- ERR` if found.

@nkakouros nkakouros mentioned this pull request Jan 16, 2018
3 tasks
@coveralls
Copy link

Coverage Status

Coverage decreased (-22.4%) to 72.675% when pulling 9f38958 on tterranigma:unbound into b8d7140 on mbland:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-22.4%) to 72.675% when pulling 0899bfe on tterranigma:unbound into b8d7140 on mbland:master.

@coveralls
Copy link

coveralls commented Jan 20, 2018

Coverage Status

Coverage decreased (-22.3%) to 72.725% when pulling 91dc1c2 on tterranigma:unbound into b8d7140 on mbland:master.

@nkakouros nkakouros changed the title Adds partial support for the unofficial bash strict mode [WIP] Adds partial support for the unofficial bash strict mode Jan 22, 2018
completion_item_reference="$completions_reference[$i]"
if [[ "${!completion_item_reference}" == "$arg" ]]; then
if [[ "${!completion_item_reference-}" == "$arg" ]]; then
unset "$completions_reference[$i]"
Copy link
Author

Choose a reason for hiding this comment

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

Here, it would make sense to me to actually reduce num_completions by one as the array indeed has one less item. But if I do this, then ./go test modules/arg-completion fails on the last test. Why is that?

@nkakouros nkakouros closed this Oct 25, 2018
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