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

Apply shellcheck to all vscodium shell scripts #1572

Merged
merged 2 commits into from
Jul 21, 2023
Merged

Apply shellcheck to all vscodium shell scripts #1572

merged 2 commits into from
Jul 21, 2023

Conversation

zpiatt
Copy link
Contributor

@zpiatt zpiatt commented Jul 21, 2023

All shell scripts within the VSCodium repository's now pass shellcheck.

  1. Fixed glob used in command name ( Unclear functions related to Shellcheck errors #1570 ).
  2. Fixed typo causing logical error piping into true ( Unclear functions related to Shellcheck errors #1570 ).
  3. Fixed options specified in getopts but not handled by case statement https://www.shellcheck.net/wiki/SC2213
  4. Handled invalid flags ( with implied continue ) in case statements ( https://www.shellcheck.net/wiki/SC2220 ).
  5. Read error codes directly (style) ( https://www.shellcheck.net/wiki/SC2181 ).
  6. Fixed all instances of reading/writing files in same pipeline (https://www.shellcheck.net/wiki/SC2094).
  7. Removed legacy back-ticks ( used $( command ) with spaces) (https://www.shellcheck.net/wiki/SC2006).
  8. Removed useless cat (https://www.shellcheck.net/wiki/SC2002).
  9. Added -r to read commands (https://www.shellcheck.net/wiki/SC2162).

Special shellcheck considerations:

  1. shellcheck recommends many style changes for both readability and consistency ( both subjective ). Where applicable I've ignored these messages to conform with the style preference of the project. One example is: https://www.shellcheck.net/wiki/SC2129
  2. shellcheck notes when it is unable to follow variables or files when sourced. When applicable, I ignored this message (https://www.shellcheck.net/wiki/SC1091) with a directive (https://github.com/koalaman/shellcheck/wiki/directive).
  3. I believe the shellcheck disable messages are a value add, because they cut down on the noise when searching for actual errors.

Other small changes:

  1. I standardized all > /dev/null 2>&1 to simply &> /dev/null. I found it both ways and chose the latter.
  2. I standardized newlines in if statements and for loops.
  3. I standardized all shebangs with #!/usr/bin/env bash.
  4. I standardized all tests with [[ ]].

@zpiatt
Copy link
Contributor Author

zpiatt commented Jul 21, 2023

I apologize, I made a note and forgot to come back to it. I found the variable PRODUCT_SKU assigned in build/windows/msi/build.sh that doesn't appear to be referenced anywhere else in the project as below:

~/git/vscodium $ grep -rn "PRODUCT_SKU"
build/windows/msi/build.sh:14:  PRODUCT_SKU="vscodium-insiders"
build/windows/msi/build.sh:21:  PRODUCT_SKU="vscodium"

@zpiatt
Copy link
Contributor Author

zpiatt commented Jul 21, 2023

I'm not certain the failed check is related to changes I've made. The workflow is attempting to download node.js 16.17.1 linux armv7l but the current LTS Linux ARMv7 is node-v18.17.0-linux-armv7l.tar. It appears the node version is being specified as 16 here: .github/workflows/insider-linux.yml. The link in the log also fails using: https://nodejs.org.

@daiyam
Copy link
Member

daiyam commented Jul 21, 2023

The variable PRODUCT_SKU can be removed.

I've re-running the failed action because it can failed at that step due to network issue (I think) from time to time...

@zpiatt
Copy link
Contributor Author

zpiatt commented Jul 21, 2023

The variable PRODUCT_SKU can be removed.

I've re-running the failed action because it can failed at that step due to network issue (I think) from time to time...

Yes, and I'll remove my erroneous "ignore" as well.

@daiyam
Copy link
Member

daiyam commented Jul 21, 2023

I've re-running the failed action because it can failed at that step due to network issue (I think) from time to time...

The action have passed.

@zpiatt
Copy link
Contributor Author

zpiatt commented Jul 21, 2023

I've re-running the failed action because it can failed at that step due to network issue (I think) from time to time...

The action have passed.

Ok, I've made the other changes. Do you want me to commit them now?

build/update_api.sh Outdated Show resolved Hide resolved
@daiyam
Copy link
Member

daiyam commented Jul 21, 2023

Ok, I've made the other changes. Do you want me to commit them now?

Yes, you can.

@daiyam
Copy link
Member

daiyam commented Jul 21, 2023

LGTM!

Thank you for the PR!

@daiyam daiyam merged commit 1c0c30e into VSCodium:master Jul 21, 2023
@zpiatt zpiatt deleted the shellcheck-applied-01 branch July 21, 2023 15:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants