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

bug: enable shellcheck & shfmt for asdf entrypoints #1397

Closed
jthegedus opened this issue Dec 23, 2022 · 3 comments · Fixed by #1480
Closed

bug: enable shellcheck & shfmt for asdf entrypoints #1397

jthegedus opened this issue Dec 23, 2022 · 3 comments · Fixed by #1480
Labels
bug priority asdf core intend to resolve soon

Comments

@jthegedus
Copy link
Contributor

jthegedus commented Dec 23, 2022

Describe the Bug

#1391 updated the scripts/*.bash to add a specific Shellcheck & shfmt commands to validate our asdf.sh and lib/*.sh files. This code was previously part of a single check, but was probably misclassifying the files as Bash, when they're seemingly intended to be POSIX.

The updated checking was introduced but disabled. We should:

  • determine if we want asdf.sh or lib/*.sh to be POSIX or Bash
  • enable these checks
  • fix the results

Steps to Reproduce

Uncomment the code in https://github.com/asdf-vm/asdf/blob/master/scripts/shellcheck.bash

# check .sh files
-# TODO(jthegedus): unlock this check later
-# TODO  shellcheck --shell sh --external-sources \
-# TODO  asdf.sh \
-# TODO  lib/*.sh
+shellcheck --shell sh --external-sources \
+  asdf.sh \
+  lib/*.sh

Uncomment the code in https://github.com/asdf-vm/asdf/blob/master/scripts/shfmt.bash

# check .sh files
-# TODO(jthegedus): unlock this check later
-# TODO  shfmt --language-dialect posix --indent 2 --diff \
-# TODO  asdf.sh \
-# TODO  lib/*.sh
+shfmt --language-dialect posix --indent 2 --diff \
+  asdf.sh \
+  lib/*.sh

Uncomment the code in https://github.com/asdf-vm/asdf/blob/master/scripts/format.bash

# check .sh files
-# TODO(jthegedus): unlock this check later
-# TODO  shfmt --language-dialect posix --indent 2 --write\
-# TODO  asdf.sh \
-# TODO  lib/*.sh
+shfmt --language-dialect posix --indent 2 --write\
+  asdf.sh \
+  lib/*.sh

Expected Behaviour

CI will run this script and reveal a bunch of issues to be fixed.

Actual Behaviour

These are currently skipped.

Environment

╭─jthegedus@art at 20:58:41
├ ~/dev/asdf
├  master
├ Last command took 596ms 
╰─λ asdf info
OS:
Linux art-win11 5.15.79.1-microsoft-standard-WSL2 #1 SMP Wed Nov 23 01:01:46 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

SHELL:
fish, version 3.3.1

ASDF VERSION:
v0.11.0-5af7625

ASDF ENVIRONMENT VARIABLES:
ASDF_DIR=/home/jthegedus/.asdf

ASDF INSTALLED PLUGINS:
bats                         https://github.com/timgluz/asdf-bats.git main 299551f
deno                         https://github.com/asdf-community/asdf-deno.git master 87442fe
firebase                     https://github.com/jthegedus/asdf-firebase.git main 4a0ac90
gcloud                       https://github.com/jthegedus/asdf-gcloud main a71ab95
java                         https://github.com/halcyon/asdf-java.git master 0720a4b
nodejs                       https://github.com/asdf-vm/asdf-nodejs.git master c9e5df4
pnpm                         https://github.com/jonathanmorley/asdf-pnpm.git master 95752a9
python                       https://github.com/danhper/asdf-python.git master 8505457
shellcheck                   https://github.com/luizm/asdf-shellcheck.git master 0f79f0b
shfmt                        https://github.com/luizm/asdf-shfmt.git master a42c5ff
v                            https://github.com/jthegedus/asdf-v main 99136a7

asdf plugins affected (if relevant)

NA

@jthegedus jthegedus added bug priority asdf core intend to resolve soon labels Dec 23, 2022
@hyperupcall
Copy link
Contributor

May I jump in and resolve this? I see you have marked the TODOs as yours and I wasn't sure if you have already started the fixes

@jthegedus
Copy link
Contributor Author

@hyperupcall Before we make code changes, I would like to get an agreement with @Stratus3D on this aspect from the OP:

determine if we want asdf.sh or lib/*.sh to be POSIX or Bash

@Stratus3D
Copy link
Member

Hi @jthegedus, sorry for the late reply here. I think *.sh files sourced by the user should be POSIX for compatibility reasons. Only files with a *.bashfile extension or files with an explicit Bash shebang line should be treated as Bash.

It looks like that is the approach you all have taken so it seems good to me 👍

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug priority asdf core intend to resolve soon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants