search

TruCol / Self-host-GitLab-CI-for-GitHub

Installs your own GitLab CI and runs it on all your GitHub repos, in a single command.
GNU Affero General Public License v3.0
4 stars 3 forks source link

Realise Google Shell Style Guide Compliance #27

Open a-t-0 opened 2 years ago

a-t-0 commented 2 years ago

Style Guide Compliance Instructions

This is the Google Shell style Guide:https://google.github.io/styleguide/shellguide.html die which can be used for Bash. To do so, one can make a list of TODO's per file/function and cross of the list of TODO's for each file/function:

The list is not a complete parse of the original Style Guide, it still contains some: <TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>. Please read the Style Guide, and write the actionable (and verifiable) TODO's per (sub)section, as a comment. Then I'll refactor that and update TODO lists below. 

- [ ] A.0 Section: STDOUT vs STDERR: All error messages should go to STDERR.
- [ ] A.1 Section: STDOUT vs STDERR: Write and use a function to print out error messages 
- [ ] A.2 Section: STDOUT vs STDERR: Verify the tests on this function still work. (In particular that they catch errors correctly).

- [ ] B.0 Section: Comments, subsection: File Header: Start each file with a description of its contents.
- [ ] B.1 Section: Comments, subsection: Function Comments: Any function that is not both obvious and short must be commented.
- [ ] B.2 Section: Comments, subsection: Function Comments: It should be possible for someone else to learn how to use your program by reading the comments without reading the code.
- [ ] B.3. Section: Comments, subsection: Function Comments: All function comments should describe the intended API behaviour using:
    - [ ] B.3.0 Description of the function.
    - [ ] B.3.1 Globals: List of global variables used and modified.
    - [ ] B.3.2 Arguments: Arguments taken.
    - [ ] B.3.3 Outputs: Output to STDOUT or STDERR.
    - [ ] B.3.4 Returns: Returned values other than the default exit status of the last command run.
- [ ] B.4 Section: Comments, subsection: Implementation Comments: Comment tricky, non-obvious, interesting or important parts of your code.
- [ ] B.5 Section: Comments, subsection: TODO Comments: Use TODO comments for code that is temporary, a short-term solution, or good-enough but not perfect.
    - [ ] B.5.1 TODOs should include the string TODO in all caps, followed by the handle of the person with the best context about the problem referenced by the TODO.

- [ ] C.1 Section: Formatting, subsection: Indentation: Indent 2 spaces. No tabs. (notepad++ ctrl+h (open replace), alt+x (enable extended search/replace) replace "\t" with:"  " (zonder de quotations)).
- [ ] C.2 Section: Formatting, subsection: Line Length: Maximum line length is 80 characters.
- [ ] C.3 Section: Formatting, subsection Pipelines: Pipelines should be split one per line if they don’t all fit on one line.
- [ ] C.4.1 Section: Formatting, subsection Loops: Put ; do and ; then on the same line as the while, for or if.
- [ ] C.4.2 Section: Formatting, subsection Loops: else should be on its own line
- [ ] C.4.3 Section: Formatting, subsection Loops: closing statements should be on their own line vertically aligned with the opening statement. 
- [ ] C.5.1 Section: Formatting, subsection Case: Indent alternatives by 2 spaces.
- [ ] C.5.2 Section: Formatting, subsection Case: ???A one-line alternative needs a space after the close parenthesis of the pattern and before the ;;.
- [ ] C.5.3 Section: Formatting, subsection Case: Long or multi-command alternatives should be split over multiple lines with the pattern, actions, and ;; on separate lines.
- [ ] C.5.4 Section: Formatting, subsection Case: The matching expressions are indented one level from the case and esac. 
- [ ] C.5.5 Section: Formatting, subsection Case:  Multiline actions are indented another level.
- [ ] C.5.6 Section: Formatting, subsection Case:  Pattern expressions should not be preceded by an open parenthesis. 
- [ ] C.5.7 Section: Formatting, subsection Case: Avoid the ;& and ;;& notations.
- [ ] C.5.8 Section: Formatting, subsection Case: 
- [ ] C.6 Section: Formatting, subsection Variable Expansion: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] C.7 Section: Formatting, subsection Quoting: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`

- [ ] D.1 Section: Features and Bugs, subsection: ShellCheck: ensure shellcheck compliance
- [ ] D.2 Section: Features and Bugs, subsection: Command Substitution: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] D.3 Section: Features and Bugs, subsection: Test, [] and [[]]: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] D.4 Section: Features and Bugs, subsection: Testing Strings: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] D.5 Section: Features and Bugs, subsection: WildCard Expansion of Filenames: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] D.6 Section: Features and Bugs, subsection: Eval: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] D.7 Section: Features and Bugs, subsection: Arrays: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] D.8 Section: Features and Bugs, subsection: Pipes to While: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] D.9 Section: Features and Bugs, subsection: Arithmetic: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`

- [ ] E.1 Section: Naming Conventions, subsection: Function Names: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] E.2 Section: Naming Conventions, subsection: Variable Names: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] E.3 Section: Naming Conventions, subsection: Constants and Environment Variable Names: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] E.4 Section: Naming Conventions, subsection: Source Filenames: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] E.5 Section: Naming Conventions, subsection: Read-only Variables: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] E.6 Section: Naming Conventions, subsection: Use Local Variables: ensure all local variables are created as `local variablename`. (With declaration separate from initialisation (so write variablename=.... on another line)).
- [ ] E.7 Section: Naming Conventions, subsection: Function Location: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] E.8 Section: Naming Conventions, subsection: : `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`

- [ ] F.1 Section: Calling COmmands, subsection: Checking Return Values: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`
- [ ] F.2 Section: Calling COmmands, subsection: Builtin Commands vs. External Commands: `<TODO: please parse the requirements from the Google Style Guide into concrete, verifiable instructions here>`

Shellcheck Compliance

Shellcheck compliance can be realised: https://github.com/koalaman/shellcheck . Below are instructions that can be used in this process. Another option was given, and that was to install the ShellCheck plugin into IntelliJ (or VScode) to directly see where the code is not ShellCheck Compliant.

  1. Open terminal.
  2. cd (change directory) to the path where you have a bash file that you want to make shellcheck compliant. e.g. 
    cd c:/some_folder/
  3. Verify it contains the file you want: e.g. dir should return: 
    the_bash_file_you_want_to_make_shellcheck_compliant.sh
  4. Type: 
    shellcheck the_bash_file_you_want_to_make_shellcheck_compliant.sh
  5. enter.
  6. Then you get a list of things that are not shellcheck compliant.
  7. Then you look up what the shellcheck issue number means.
  8. Then you do the magic=resolve it intelligently to make it shellcheck compliant. For example, put variable inside double quotes. 7.1 If making it shellcheck compliant would require an undesired change of the code, you could include a message for shellcheck to ignore that specific incompliance https://github.com/koalaman/shellcheck/wiki/Ignore: 
    # shellcheck disable=SC2116
hash=$(echo ${hash})    # trim spaces
  9. Then you run shellcheck again on the same file to verify you have made it fully shellcheck compliant =no more warnings/errors.

Ideally, a contributer:

  1. Completes the Google Style Guide conversion into concrete, verifiable TODO's.
  2. Posts a comment, claiming which file they will improve.
  3. Create a fork (per file).
  4. Make the file compliant according to the TODO's.
  5. Send a Pull Request (per file).

The list of files is specified below, and for those who like checking off small tasks, all tasks for each file have been included in the comments below.

a-t-0 commented 2 years ago

Most relevant for improvement:

For the tests, check in test.sh which tests work, and start with those.

mohitsaxenaknoldus commented 2 years ago

I'd like to start with multiple files at once. Are all the above files available?

a-t-0 commented 2 years ago

Awesome! All except for the 

 src/helper_gitlab_modify.sh

are available. (Here you can see the work on that file:https://github.com/Simple-Setup/Self-host-GitLab-Server-and-Runner-CI/pull/37)

mohitsaxenaknoldus commented 2 years ago

Ok, thank you! I am on it! btw, there is an easier way to do this. What I do is I've installed Shellcheck plugin on Intellij and it tells me exactly where the problem is, with the SC code so it becomes easier and you don't have to manually compare two files and edit those.

a-t-0 commented 2 years ago

Thank's for the tip! I'm starting to feel like a fossil desperately clamping onto old habits I have, using Notepad++ as editor, while the evidence in favour of using an IDE like VSCode, or in this case Intellij is mounting!

I will take it into consideration!

mohitsaxenaknoldus commented 2 years ago

PR: https://github.com/Simple-Setup/Self-host-GitLab-Server-and-Runner-CI/pull/41/

a-t-0 commented 2 years ago

Most relevant for improvement (In order of importance):

a-t-0 commented 2 years ago
a-t-0 commented 2 years ago
a-t-0 commented 2 years ago

For the tests, check in test.sh which tests work, and start with those.

a-t-0 commented 2 years ago
mohitsaxenaknoldus commented 2 years ago

I'll raise a PR tomorrow to address these things.