search

reviewdog / action-shellcheck

Run shellcheck with reviewdog
https://github.com/marketplace?type=actions&query=reviewdog
MIT License
102 stars 20 forks source link

use grep instead of find to select files for shellcheck #5

Closed fbartels closed 4 years ago

fbartels commented 4 years ago

fixes #2

Signed-off-by: Felix Bartels felix@host-consultants.de

fbartels commented 4 years ago

@haya14busa ok, I think I am good for the moment.

PS: in the output of the action it says reviwedog: Reporting results for "shellcheck" is that a typo in reviewdog?

haya14busa commented 4 years ago

PS: in the output of the action it says reviwedog: Reporting results for "shellcheck" is that a type on reviewdog?

Thanks for the catch! It's typo. I'll fix it.

fbartels commented 4 years ago

Can you add a flag to use grep & shebang instead of removing find command usage?

I had thought of that before, but to me it seemed like this would duplicate a lot of code. the leanes way possible I could think of was putting the file listing into a bash function and then pipe the result to shellcheck (and the function could have a if for grep vs. find). I ultimately decided against it, since it did not seem like worth the effort, since ...

Not all shell scripts have shebang.

I think this is just educating users into the wrong direction. What valid reason could there be not to have a shebang? is that the only reason find could be preferred over grep?

haya14busa commented 4 years ago
  1. reviewdog itself is not an educational application, if all shellscript should have shebang, shellcheck should report it and reviewdog should not enforce it.
  2. In my understanding shell script files used as a library don't need to have shebang as per shebang's purpose for example.
haya14busa commented 4 years ago

pseudo code

FILES=""
if pattern is not empty
  FILES=$(find ...)
else
  FILES=$(grep ...)
fi

shellcheck ... $FILES | reviewdog ...
fbartels commented 4 years ago

In my understanding shell script files used as a library don't need to have shebang as per shebang's purpose for example.

You mean when the file is sourced by another script? That's true in that case there does not need to be a shebang, but even in the original version of your script you were using "--external-sources" to tell shellcheck to also check sourced files.

But the both of us have obviously different perspectives on that matter, which is of course allowed. I think I'll just maintain my own version of this repo to not introduce too many breaking changes for existing users of it.

lsgunth commented 4 years ago

Sigh. Seems like the correct answer is to do both all files that contain a shebang plus any pattern the user specifies. If the user doesn't specify a pattern and the file has no shebang, there's nothing we can do about it. If they want us to check all .sh files we should do that regardless of whether there's a shebang.

This should easily be doable without too much fuss or code duplication with something like:

(grep "${GREP_ARGS[@]}" ; find "${FIND_ARGS[@]}") | sort -u | xargs shellcheck ...

Sadly I don't have time to implement or test this myself.

alan-barzilay commented 3 years ago

@haya14busa sorry if I'm asking you to repeat yourself, but what exactly is your position on this matter? is @lsgunth suggestion acceptable? I wouldn't mind implementing a PR if I can get some pointers, this would be a huge boon for the project I'm currently developing as part of GSoC since not being able to specify multiple patterns is a huge issue for us