MycroftAI / mycroft-core

Mycroft Core, the Mycroft Artificial Intelligence platform.
https://mycroft.ai
Apache License 2.0
6.51k stars 1.27k forks source link

Refactor/shellcheck bin #3019

Closed forslund closed 2 years ago

forslund commented 2 years ago

Description

Adds shellcheck stage to github actions as discussed in #3015 checking the shellscripts in bin. This should be extended to also check the dev-setup.sh and scripts folder etc. but this is a start.

This also fixes the issues detected by shellcheck in the checked folder, the biggest changes were in mycroft-config.

The current check excludes the following checks:

How to test

Ensure github action Shellcheck runs ok.

Contributor license agreement signed?

CLA [ x]

devops-mycroft commented 2 years ago

Voight Kampff Integration Test Succeeded (Results)

forslund commented 2 years ago

Manual tests are most welcome, I tried to exercise all updated scripts but may well have missed things. Especially in the more complex scripts.

codecov-commenter commented 2 years ago

Codecov Report

Merging #3019 (6c5556c) into dev (0247b3a) will increase coverage by 0.00%. The diff coverage is n/a.

:exclamation: Current head 6c5556c differs from pull request most recent head 1f93381. Consider uploading reports for the commit 1f93381 to get more accurate results Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #3019   +/-   ##
=======================================
  Coverage   53.03%   53.04%           
=======================================
  Files         123      123           
  Lines       11170    11170           
=======================================
+ Hits         5924     5925    +1     
+ Misses       5246     5245    -1     
Impacted Files Coverage Δ
mycroft/client/speech/listener.py 48.39% <0.00%> (+0.35%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0247b3a...1f93381. Read the comment docs.

PureTryOut commented 2 years ago

The current check excludes the following checks: SC1091: Avoids errors when shellcheck can't find sourced file SC2034: Unused variables, for example colors that aren't used yet SC2012: use of ls, from what I can see in our case this is fine (wc -l)

Please let shellcheck ignore those errors with a directive if you want to ignore those errors @forslund. I was wondering why I was still seeing warnings when running shellcheck locally lol.

forslund commented 2 years ago

I decided against using the directive in the script files since it in my opinion it cluttered up the files unnecessarily but I can make that change if it's preferred.

PureTryOut commented 2 years ago

I disagree with it making it cluttered, and right now Shellcheck still spits out warnings which is ugly and imo worrying. There are fine reasons to ignore some warnings but shellcheck should really be told to ignore them. Now people might get a "oh this warning is like one of those others we ignore, I can ignore this one too" feeling while it might this time actually valid complain. Let's be explicit that we allow these particular "mistakes".

forslund commented 2 years ago

Your reasoning is valid too I'll make an update to this effect