fix: ignore non-executable files in check_shellscript_set_options.py
#98
Conversation
| @@ -55,8 +65,8 @@ def _are_shell_files_valid(shell_files: list[Path], expected_options: str) -> bo | |||
| def main(argv: Sequence[str] | None = None) -> int: | |||
| args = parse_arguments(argv) | |||
|
|
|||
| bash_files, sh_files = _separate_bash_from_sh_files(args.filenames) | |||
| are_all_files_valid = _are_shell_files_valid(bash_files, "set -euxo pipefail") | |||
| are_all_files_valid, bash_files, sh_files = _separate_bash_from_sh_files(args.filenames) | |||
There was a problem hiding this comment.
Instead of extending the separation function which should be only about separation, I would suggest to do the filtering here first
| are_all_files_valid, bash_files, sh_files = _separate_bash_from_sh_files(args.filenames) | |
| executable_files = [filename for filename in args.filenames if _is_executable(filename)] | |
| are_all_files_valid, bash_files, sh_files = _separate_bash_from_sh_files(executable_files) |
If you want to unit test this, you could also move the list comprehension into a small function
There was a problem hiding this comment.
@cbachhuber I was too slow, but could be a follow up PR. I still think that separation and executable filtering are 2 different tasks.
There was a problem hiding this comment.
The logic is more intricate. We ultimately want to find out if the script is bash or sh. But we don't necessarily care about if it is executable or has a shebang. We only use the shebang to determine the language.
So if we now filter executable first, we would filter out files that are non-executable, but have a shebang and would have been flagged before.
There was a problem hiding this comment.
But scripts that are not executable could be considered as "libraries" and don't need a shebang
There was a problem hiding this comment.
But we still want the set ... for them right?
There was a problem hiding this comment.
If they are considered as libraries I think they get what the executing script defines
No description provided.