Reconsider checking the apps scripts with ShellCheck

Contribute Room Apps packaging
1

Creating a new topic because @moutonjr1’s Best pracices: using ShellCheck to sanitize scripts has been closed.

I’d propose reconsidering using ShellCheck. I believe it could save time fixing issues that could have been automatically prevented, like this one:

and let contributors focus their limited resources on other areas of the project :muscle:

5 Likes
2

(Related discussion : add shellcheck config by OniriCorpe · Pull Request #220 · YunoHost/example_ynh · GitHub )

(Personal opinion from a few days ago on the chat

yyyyah heuarg i should get more familiar with shellcheck, i was kind of like “meh it’s gonna spit out so many errors” and i’m like “uuuugh yet another file” but not really a good argument

and then i’m like zhgfmvflfv we should really have packaging v3 to drastically reduce the amount of bash code again rather than micro-managing the current syntax :expressionless:

but maybe displaying the shellcheck results in the linter could be gud, should give this a try

Basically yes but if we suddently add this in the linter this may be overwhelming idk, should give this a try

2 Likes
3

tl;dr the issues are :

  • the helpers have some weird syntaxes that shellchecks struggles (kinda easily fixable)
  • shellcheck doesn’t really like non-constant source, so that doesn’t work : https://github.com/YunoHost/yunohost/blob/4897f72974fff30b168a40bf55bf917395cca4cc/share/helpers#L6
  • the manifest variables aren’t defined in bash per se, we need some tweaks to tell shellcheck “yea yea, those variables are defined don’t worry”. (We could actually silence “undefined variables” warnings)
  • the variables used in templates (__MYVAR__) aren’t used in bash per se either, so shellcheck thinks the variables are unused → IMHO we could just silence “unused variable” warnings.