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

ci: add pre-commit hook to remove trailing whitespaces #73

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flavono123
Copy link
Collaborator

Goal

Descriptions

  • add pre-commit hook to remove trailing whitespaces
  • flow
    • collect the staged files
    • check if any of these files have whitespace issues
    • if so, cleanup process
      • loop through the files and apply git-stripspace

Requirements

  • git config core.hooksPath .githooks to activate the hook
  • git config core.quotepath false for non-ascii paths
    • also need to be documented(where?)

Test

  • trailing spaces in following diff are cleaned up
    • works on all type of file even for codes, not only for markdowns(docs)
diff --git a/.githooks/pre-commit b/.githooks/pre-commit
new file mode 100644
index 0000000..4d25c49
--- /dev/null
+++ b/.githooks/pre-commit
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+# Temporary file to store the list of files that need cleanup
+TEMP_FILE=$(mktemp)
+
+# Collect staged files with potential whitespace issues
+git diff --cached --name-only --diff-filter=ACM > "$TEMP_FILE"
+
+# Check if any of these files have whitespace issues
+NEEDS_CLEANUP=false
+while read -r FILE; do
+  if ! git diff --cached --check -- "$FILE"; then
+    NEEDS_CLEANUP=true
+    break
+  fi
+done < "$TEMP_FILE"
+
+# Cleanup process
+if [ "$NEEDS_CLEANUP" = true ]; then
+  # Loop through the files and apply git-stripspace
+  while read -r FILE; do
+    git show ":$FILE" | git stripspace > "$FILE"
+    git add "$FILE"
+  done < "$TEMP_FILE"
+
+  # Inform the user
+  echo 'Cleanup trailing whitespaces is done'
+fi
+
+# Clean up - remove the temporary file
+rm "$TEMP_FILE"
+
+if [ "$NEEDS_CLEANUP" = false ]; then
+  echo "no need to cleanup"
+fi
+# Continue with the original commit
+exit 0
diff --git "a/locales/ko/\355\205\234\355\224\214\353\246\277/\352\262\260\354\240\225 \352\270\260\353\241\235 \355\205\234\355\224\214\353\246\277\353\263\204 edgex/index.md" "b/locales/ko/\355\205\234\355\224\214\353\246\277/\352\262\260\354\240\225 \352\270\260\353\241\235 \355\205\234\355\224\214\353\246\277\353\263\204 edgex/index.md"
index f4c35a5..bdbed75 100644
--- "a/locales/ko/\355\205\234\355\224\214\353\246\277/\352\262\260\354\240\225 \352\270\260\353\241\235 \355\205\234\355\224\214\353\246\277\353\263\204 edgex/index.md"
+++ "b/locales/ko/\355\205\234\355\224\214\353\246\277/\352\262\260\354\240\225 \352\270\260\353\241\235 \355\205\234\355\224\214\353\246\277\353\263\204 edgex/index.md"
@@ -5,7 +5,7 @@ EdgeX Foundry의 ADR 템플릿입니다.
 출처: https://docs.edgexfoundry.org/2.3/design/adr/template/

-### 제출자
+### 제출자

 ADR 제출자를 나열합니다.

Signed-off-by: flavono123 flavono123@gmail.com

\### Goal

- reduce the tedious chore of removing trailing whitespaces
- ref. joelparkerhenderson#69 (comment)

\### Descriptions

- add pre-commit hook to remove trailing whitespaces
- flow
  - collect the staged files
  - check if any of these files have whitespace issues
  - if so, cleanup process
    - loop through the files and apply git-stripspace

\### Requirements

- `git config core.hooksPath .githooks` to activate the hook
- `git config core.quotepath false` for non-ascii paths
  - also need to be documented(where?)

\### Test
- trailing spaces in following diff are cleaned up
  - works on all type of file even for codes, not only for markdowns(docs)
```diff
diff --git a/.githooks/pre-commit b/.githooks/pre-commit
new file mode 100644
index 0000000..4d25c49
--- /dev/null
+++ b/.githooks/pre-commit
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+# Temporary file to store the list of files that need cleanup
+TEMP_FILE=$(mktemp)
+
+# Collect staged files with potential whitespace issues
+git diff --cached --name-only --diff-filter=ACM > "$TEMP_FILE"
+
+# Check if any of these files have whitespace issues
+NEEDS_CLEANUP=false
+while read -r FILE; do
+  if ! git diff --cached --check -- "$FILE"; then
+    NEEDS_CLEANUP=true
+    break
+  fi
+done < "$TEMP_FILE"
+
+# Cleanup process
+if [ "$NEEDS_CLEANUP" = true ]; then
+  # Loop through the files and apply git-stripspace
+  while read -r FILE; do
+    git show ":$FILE" | git stripspace > "$FILE"
+    git add "$FILE"
+  done < "$TEMP_FILE"
+
+  # Inform the user
+  echo 'Cleanup trailing whitespaces is done'
+fi
+
+# Clean up - remove the temporary file
+rm "$TEMP_FILE"
+
+if [ "$NEEDS_CLEANUP" = false ]; then
+  echo "no need to cleanup"
+fi
+# Continue with the original commit
+exit 0
diff --git "a/locales/ko/\355\205\234\355\224\214\353\246\277/\352\262\260\354\240\225 \352\270\260\353\241\235 \355\205\234\355\224\214\353\246\277\353\263\204 edgex/index.md" "b/locales/ko/\355\205\234\355\224\214\353\246\277/\352\262\260\354\240\225 \352\270\260\353\241\235 \355\205\234\355\224\214\353\246\277\353\263\204 edgex/index.md"
index f4c35a5..bdbed75 100644
--- "a/locales/ko/\355\205\234\355\224\214\353\246\277/\352\262\260\354\240\225 \352\270\260\353\241\235 \355\205\234\355\224\214\353\246\277\353\263\204 edgex/index.md"
+++ "b/locales/ko/\355\205\234\355\224\214\353\246\277/\352\262\260\354\240\225 \352\270\260\353\241\235 \355\205\234\355\224\214\353\246\277\353\263\204 edgex/index.md"
@@ -5,7 +5,7 @@ EdgeX Foundry의 ADR 템플릿입니다.
 출처: https://docs.edgexfoundry.org/2.3/design/adr/template/

-### 제출자
+### 제출자

 ADR 제출자를 나열합니다.

```

Signed-off-by: flavono123 <flavono123@gmail.com>
@flavono123
Copy link
Collaborator Author

a suggestion for #69 (comment) by just creating this PR

@joelparkerhenderson
Copy link
Owner

joelparkerhenderson commented Nov 14, 2023

Good idea. A few questions....

Can this kind of proofreading and changing have more capabilities?

Can the code be minimized in shell, by calling on installed software, such as installed on a CI server and/or GitHub actions and/or the developer's system?

Do you foresee what you're aiming for generally heading along the direction of a formatter and/or linter?

A few questions about the shell code...

Should this be ACMR?

--diff-filter=ACM 

I believe that stripspace is intended mostly for metadata, and there's a different whitespace=fix for committed files...?

git stripspace 

Ideally can a successful run be silent, akin to the Unix convention of silent success?

-if [ "$NEEDS_CLEANUP" = false ]; then
-  echo "no need to cleanup"
-fi

Can it run with guard flags, such as set -euf? Such as this:

Can you trap the mktemp? Because it's a good idea to trap an exception, such as this:

Could it be worthwhile to look for a longer-term larger-scope solution to make your idea even more powerful and valuable? Here are some that turned up when I looked... do any of these look like they can give you more capabilities?

Overall, my opinion is your code is fine to commit and use. I favor bias for action, and you're doing action. I appreciate you!

@flavono123
Copy link
Collaborator Author

thank you for valuable and various points.
but by answering the last one, that would resolve others too i think.
and totally agree with the idea, using "a longer-term larger-scope solution", open-source standards, in other words, rather than the custom code snippet should be maintained.

in a nutshell, in advance, i prefer/think the choice is github action + megalinter:

the matter is pricing, no for megalinter but github action.
but personal free tier is enough i guess:
image
https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions#included-storage-and-minutes

@joelparkerhenderson
Copy link
Owner

joelparkerhenderson commented Nov 15, 2023

Excellent research! Yes GitHub Action + MegaLinter sounds good. And I expect that the pricing on the free tier is OK.

If you want to try it, go ahead on this whole repo.

flavono123 added a commit to flavono123/architecture-decision-record that referenced this pull request Nov 17, 2023
- discussed and researched in joelparkerhenderson#73 (comment)
- set config to lint only markdownlint
  - excluding markdown-link-check and markdown-table-formatter
- ignore reports dir
  - run freely in each local
  - or cached in CI is enough
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.

2 participants