[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #979: Use clang-format instead of cpplint

[GitBox <gi...@apache.org>]

tisonkun commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993208652


##########
x.py:
##########
@@ -74,7 +74,7 @@ def run_pipe(*args: str, msg: Optional[str]=None, verbose: bool=False, **kwargs:
 def find_command(command: str, msg: Optional[str]=None) -> str:
     return run_pipe("which", command, msg=msg).read().strip()
 
-def check_version(current: str, required: Tuple[int, int, int], prog_name: Optional[str] = None) -> None:
+def check_version(current: str, required: Tuple[int, int, int], prog_name: Optional[str] = None, exact: bool = False) -> None:

Review Comment:
   This parameter is not used?



##########
x.py:
##########
@@ -262,11 +285,13 @@ def test_go(dir: str, rest: List[str]) -> None:
         help="Check or lint source code")
     parser_check.set_defaults(func=parser_check.print_help)
     parser_check_subparsers = parser_check.add_subparsers()
-    parser_check_cpplint = parser_check_subparsers.add_parser(
-        'cpplint',
-        description="Lint code with cpplint (https://github.com/cpplint/cpplint)",
-        help="Lint code with cpplint (https://github.com/cpplint/cpplint)")
-    parser_check_cpplint.set_defaults(func=cpplint)
+    parser_check_format = parser_check_subparsers.add_parser(
+        'format',
+        description="Format source code",
+        help="Format source code")
+    parser_check_format.set_defaults(func=check_format)
+    parser_check_format.add_argument('--clang-format-path', default='clang-format', help="path of clang-format used to check source")
+    parser_check_format.add_argument('-i', default=False, action='store_true', help="format source files")

Review Comment:
   A check should not change files. Perhaps you add both `./x.py check clang-format` and `./x.py fix clang-format`.



##########
x.py:
##########
@@ -116,14 +116,37 @@ def build(dir: str, jobs: int, ghproxy: bool, ninja: bool, unittest: bool, compi
         target.append("unittest")
     run(cmake, "--build", ".", f"-j{jobs}", "-t", *target, verbose=True, cwd=dir)
 
-def cpplint() -> None:
-    command = find_command("cpplint", msg="cpplint is required")
-    options = ["--linelength=120", "--filter=-build/include_subdir,-legal/copyright,-build/c++11"]
-    sources = [*glob("src/*.h"), *glob("src/*.cc")]
-    run(command, *options, *sources, verbose=True)
+def get_source_files() -> List[str]:
+    return [
+        *glob("src/**/*.h", recursive=True),
+        *glob("src/**/*.cc", recursive=True),
+        *glob("tests/cppunit/**/*.h", recursive=True),
+        *glob("tests/cppunit/**/*.cc", recursive=True),
+        *glob("tools/kvrocks2redis/**/*.h", recursive=True),
+        *glob("tools/kvrocks2redis/**/*.cc", recursive=True),
+    ]
+
+def check_format(clang_format_path: str, i: bool) -> None:
+    print("WARNING: We use clang-format 12 in CI,\n"
+        "  so we recommend that you also use this version locally to avoid inconsistencies.\n"
+        "  You can install it from your package manager (usually in clang-12 package)\n"
+        "  or download it from https://github.com/llvm/llvm-project/releases/tag/llvmorg-12.0.1")

Review Comment:
   Shall we always show warning?



##########
.github/workflows/kvrocks.yaml:
##########
@@ -38,27 +38,35 @@ jobs:
 
   lint:
     name: Lint and check code
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-20.04
     steps:
       - name: Checkout Code Base
         uses: actions/checkout@v3
       - name: Install Check Tools
         run: |
           sudo apt update
           sudo apt install -y cppcheck
-      - name: Setup Python
-        uses: actions/setup-python@v4
-        with:
-          python-version: 3.x
+          sudo apt install -y clang-format-12
       - name: Setup Go
         uses: actions/setup-go@v3
         with:
           go-version: 1.19.x
-      - name: Setup dependencies
-        run: pip install cpplint==1.6.1
+      - name: Check code format
+        id: check-format
+        run: ./x.py check format --clang-format-path clang-format-12
+      - name: Format code
+        if: always() && steps.check-format.outcome != 'success'
+        run: |
+          ./x.py check format -i --clang-format-path clang-format-12
+          git diff -p > clang-format.patch
+          cat clang-format.patch
+      - name: Upload format patch
+        uses: actions/upload-artifact@v3
+        if: always() && steps.check-format.outcome != 'success'
+        with:
+          path: clang-format.patch

Review Comment:
   No need. I'd prefer add a CI summary or annotation to ask the user perform the format step.
   
   * https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#adding-a-job-summary
   * https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#example-creating-an-annotation-for-an-error



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org