Skip to content

Disable SC2002 -- useless use of cat, and add .shellcheckrc file #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pdewilde
Copy link
Contributor

@pdewilde pdewilde commented Apr 18, 2025

Arguably cat file | command is easier to read, as information flows from left to right.

This is especially true from something like jq, where the format is

jq [some possibly quite long query string] file where the file is the last positional argument.

rc file download uses pattern from yamllint action

@pdewilde pdewilde requested a review from a team as a code owner April 18, 2025 21:25
@pdewilde pdewilde enabled auto-merge (squash) April 18, 2025 21:28
Copy link
Contributor

@sethvargo sethvargo left a comment

Choose a reason for hiding this comment

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

I firmly disagree, and this isn't just a style thing, it's also a real performance implication for large files.

I also don't think we need an entire shellcheckrc file to disable a single rule.

@pdewilde
Copy link
Contributor Author

I firmly disagree, and this isn't just a style thing, it's also a real performance implication for large files.

pdewilde@pcd:/tmp$ ls -lh crand.csv
-rw-r--r-- 1 pdewilde primarygroup 37G Apr 21 12:31 crand.csv

pdewilde@pcd:/tmp$ time (cat crand.csv | grep -o "12346")

real    0m35.759s
user    0m29.357s
sys     0m22.164s
pdewilde@pcd:/tmp$ time (grep -o "12346" crand.csv)

real    0m33.275s
user    0m28.042s
sys     0m5.228s
pdewilde@pcd:/tmp$ time (grep -o "12346" < crand.csv)

real    0m32.921s
user    0m27.812s
sys     0m5.103s

It definitely has some additional load on the kernel, though wall clock time difference even for a 37GiB file is negligible. I could see an argument for an application that does a lot of random access, but that generally isn't what you are doing when piping together a bunch of commands.

@sethvargo
Copy link
Contributor

If there was a reasonable way to disable just this rule, I'd be fine with it, but I'm somewhat opposed to taking on all this additional complexity just to exclude it.

@pdewilde
Copy link
Contributor Author

I think it can be disabled via CLI flag. I added the rc file to match the convention used by the other linters, assuming we may want more customization in the future.

@sethvargo
Copy link
Contributor

I guess I'm indifferent. I'll defer to @dcreey on what he wants to do here.

@@ -19,11 +19,56 @@ inputs:
description: 'File or directory containing shell files to lint.'
Copy link
Contributor

Choose a reason for hiding this comment

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

We will need to figure out how to get actionlint to pick up this config too.

@dcreey
Copy link
Contributor

dcreey commented Apr 22, 2025

I guess I'm indifferent. I'll defer to @dcreey on what he wants to do here.

I like the idea of being consistent with how we configure actionlint shell linting. Right now that is via cli flags. If we can figure out how to do both from config file then that would be ok. A configuration file is a bit nicer as configuration gets more complex.

Copy link

@mattrandallbecker mattrandallbecker left a comment

Choose a reason for hiding this comment

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

Appreciate you actually checking the performance difference @pdewilde

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

Successfully merging this pull request may close these issues.

4 participants