Skip to content

fix: ignore non-executable files in check_shellscript_set_options.py#98

Merged
lukasoyen merged 1 commit into
masterfrom
fix-check_shellscript_set_options
Apr 25, 2025
Merged

fix: ignore non-executable files in check_shellscript_set_options.py#98
lukasoyen merged 1 commit into
masterfrom
fix-check_shellscript_set_options

Conversation

@lukasoyen
Copy link
Copy Markdown
Contributor

No description provided.

@lukasoyen lukasoyen requested review from cbachhuber and hofbi April 25, 2025 09:57
@lukasoyen lukasoyen merged commit 915de41 into master Apr 25, 2025
7 checks passed
@lukasoyen lukasoyen deleted the fix-check_shellscript_set_options branch April 25, 2025 10:02

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)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of extending the separation function which should be only about separation, I would suggest to do the filtering here first

Suggested change
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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbachhuber I was too slow, but could be a follow up PR. I still think that separation and executable filtering are 2 different tasks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But scripts that are not executable could be considered as "libraries" and don't need a shebang

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we still want the set ... for them right?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they are considered as libraries I think they get what the executing script defines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants