Skip to content

Conversation

@Stratus3D
Copy link
Member

Summary

  • Add echo to list of banned commands.
  • Replace echo calls with calls to printf.
  • Correct existing printf calls that were generating ShellCheck warnings.

Fixes: #781

@Stratus3D Stratus3D requested a review from a team as a code owner September 21, 2020 16:24
@Stratus3D Stratus3D changed the title Tb/ban echo Ban echo command Sep 21, 2020
printf "%s:\\n%s\\n\\n" "SHELL" "$($SHELL --version)"
printf "%s:\\n%s\\n\\n" "ASDF VERSION" "$(asdf_version)"
printf "%s:\\n%s\\n\\n" "ASDF ENVIRONMENT VARIABLES" "$(env | grep -E "ASDF_DIR|ASDF_DATA_DIR|ASDF_CONFIG_FILE|ASDF_DEFAULT_TOOL_VERSIONS_FILENAME")"
printf "%s:\\n%s\\n\\n" "ASDF INSTALLED PLUGINS" "$(asdf plugin list --urls)"
Copy link
Member Author

Choose a reason for hiding this comment

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

ShellCheck recommends \\n for double quoted strings.

Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

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

LGTM

@jthegedus
Copy link
Contributor

jthegedus commented Sep 21, 2020

As a general point of discussion, what are your thoughts on:

  1. first param to printf always being formatting only (no content)? EG:

    -printf "asdf is self upgrading shims to new asdf exec ...\\n"
    +printf "%s\\n" "asdf is self upgrading shims to new asdf exec ..."

    I try not to be dogmatic about these things

  2. re-using a formatting variable for many printf statements?

    +format="%s %s\\n"
    -printf "%s %s\\n" "$asdf_cmd_dir/command-$1.bash" 2
    +printf "$format" "$asdf_cmd_dir/command-$1.bash" 2
    ...
    -printf "%s %s\\n" "$asdf_cmd_dir/command-version.bash" 2
    +printf "$format" "$asdf_cmd_dir/command-version.bash" 2

    Shellcheck does warn against this, but it's docs do mention explicitly ignoring for format reuse.

@jthegedus jthegedus changed the title Ban echo command chore: ban echo command Sep 21, 2020
@jthegedus jthegedus merged commit 8bbefba into master Sep 21, 2020
@jthegedus jthegedus deleted the tb/ban-echo branch September 21, 2020 22:27
@Stratus3D
Copy link
Member Author

  1. first param to printf always being formatting only (no content)? EG:

Either way is fine with me. Assuming the format string doesn't contain anything that would be interpreted as special characters by printf I think including text in it is ok. In the programming languages I use, I typically hardcode strings of text in the format string, and only use placeholders for values that are dynamic (ie variables).

  1. re-using a formatting variable for many printf statements?

If a pattern is repeated in several places, it may make sense to encapsulate the print in a function. Something like:

function command_output() {
  printf "%s %s\\n" "$1" "$2"
}

command_output "$asdf_cmd_dir/command-version.bash" 2

Although I think in cases like this is it overkill (format string is just item, space, item, newline).

@jthegedus
Copy link
Contributor

I disagree on 2 as too often I see the function gets co-opted to do more work and often moved further out of context into another file which exacerbates the problem. Thanks for the food for thought 👍

delgurth added a commit to delgurth/asdf-java that referenced this pull request Apr 5, 2021
With the help of BellLabs (bell-sw/Liberica#42) we now also have Liberica macOS integration support. It doesn't work for all versions we can install, so it logs a warning in case it's not possible to use their "trick".

For this to work we depend on @joschi java-metadata project. This project provides the shasum (and with that the check if the .pkg version exists).

Other things done:
- Fail if commands fail
- Fixed unzip check
- All error messages go to stderr
- Aligned indent to 4 spaces
- Removed all echo's in favor of printf in line with asdf-vm/asdf#806
- Use the bash [[ conditions
delgurth added a commit to delgurth/asdf-java that referenced this pull request Apr 6, 2021
With the help of BellLabs (bell-sw/Liberica#42) we now also have Liberica macOS integration support. It doesn't work for all versions we can install, so it logs a warning in case it's not possible to use their "trick".

For this to work we depend on @joschi java-metadata project. This project provides the shasum (and with that the check if the .pkg version exists).

Other things done:
- Fail if commands fail
- Fixed unzip check
- All error messages go to stderr
- Aligned indent to 4 spaces
- Removed all echo's in favor of printf in line with asdf-vm/asdf#806
- Use the bash [[ conditions
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.

Consider Banning echo command

3 participants