Skip to content
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

Update to use the Google shell styleguide #268

Open
evelikov opened this issue Oct 26, 2022 · 2 comments
Open

Update to use the Google shell styleguide #268

evelikov opened this issue Oct 26, 2022 · 2 comments

Comments

@evelikov
Copy link
Collaborator

Due to its age (et al) dkms has various styles mixed in. Most prominently - it does not use safe(r) bash constructs line [[ and ((, set -euo pipefail, while in other cases the code is borderline bonkers.

Please keep changes separated logically and do not clump everything into a mega-commit or two.

Ideally we'll also get a CI stage/action that ensures these are consistent, somewhat at least. Can be done alongside the proposed shellcheck #267 or via other means.

@siddhpant
Copy link

Maybe the Linux kernel style (like using tabs etc.) should be followed since this is something related to it?

Also, maybe the huge bash script could be split into multiple files for modularity... 80 char limitation will make the script code longer, so if the file is split, reading might become easier. Bash's source can "import" functions from other files.

@evelikov
Copy link
Collaborator Author

I don't understand what you mean - kernel is written in C, so their styleguide follows that, while dkms is in shell/bash. Sounds like trying to push a square peg through a round hole, with this suggestion.

Yes, dkms is big, but chunking it into smaller pieces mostly increases the odds of breaking things. Not unless we get an order of magnitude more tests. Nevertheless - that's not the concern raised here.

AllKind added a commit to AllKind/dkms_githubfork that referenced this issue Dec 15, 2023
In regard of: dell#268.
Replaced all instances of `[ expr ]' I could find with
`[[ expr ]]', or `(( expr ))'.
Also removed the now obsolete quoting.
Some unnecessary grouped conditional commands have been
consolidated.

Signed-off-by: Mart Frauenlob <[email protected]>
AllKind added a commit to AllKind/dkms_githubfork that referenced this issue Dec 15, 2023
In regard of: dell#268.
Replaced all instances of `[ expr ]' I could find with
`[[ expr ]]', or `(( expr ))'.
Also removed the now obsolete quoting.
Some unnecessary grouped conditional commands have been
consolidated.

Signed-off-by: Mart Frauenlob <[email protected]>
evelikov pushed a commit that referenced this issue Jan 31, 2024
In regard of: #268.
Replaced all instances of `[ expr ]' I could find with
`[[ expr ]]', or `(( expr ))'.
Also removed the now obsolete quoting.
Some unnecessary grouped conditional commands have been
consolidated.

Signed-off-by: Mart Frauenlob <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants