You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/10/12 09:06:13 UTC
[GitHub] [incubator-kvrocks] PragmaTwice opened a new pull request, #979: Use clang-format instead of cpplint
PragmaTwice opened a new pull request, #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979
It closes the clang-format part of #800.
--
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
[GitHub] [incubator-kvrocks] torwig commented on pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#issuecomment-1275904460
@PragmaTwice Good job!
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993308097
##########
.clang-format:
##########
@@ -0,0 +1,200 @@
+---
+Language: Cpp
+# BasedOnStyle: Google
Review Comment:
Generated? Could you share how you gain this config file?
--
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
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993434719
##########
.clang-format:
##########
@@ -0,0 +1,200 @@
+---
+Language: Cpp
+# BasedOnStyle: Google
Review Comment:
I also think 120 is enough, we should rarely have too long lines?
--
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
[GitHub] [incubator-kvrocks] torwig commented on pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#issuecomment-1276000118
@tisonkun Perhaps I'm too old, but I do "squashing" by myself :)
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993249191
##########
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:
done
##########
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:
done
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993408831
##########
x.py:
##########
@@ -116,14 +119,43 @@ 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, fix: bool = False) -> None:
+ command = find_command(clang_format_path, msg="clang-format is required")
+
+ version_res = run_pipe(command, '--version').read().strip()
+ version_str = re.search(r'version\s+((?:\w|\.)+)', version_res).group(1)
+
+ version = check_version(version_str, CLANG_FORMAT_REQUIRED_VERSION, "clang-format")
+
+ if version[0] > 12:
+ raise RuntimeError("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:
done
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993307478
##########
.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:
Yep. I noticed that you upload a patch file instead of pushing a commit. Then it's great to contain!
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993411387
##########
x.py:
##########
@@ -116,14 +119,43 @@ 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, fix: bool = False) -> None:
Review Comment:
done
##########
x.py:
##########
@@ -256,17 +288,25 @@ def test_go(dir: str, rest: List[str]) -> None:
subparsers = parser.add_subparsers()
+ parser_format = subparsers.add_parser(
+ 'format',
+ description="Format source code",
+ help="Format source code")
+ parser_format.set_defaults(func=lambda **args: check_format(**args, fix=True))
+ parser_format.add_argument('--clang-format-path', default='clang-format', help="path of clang-format used to check source")
+
parser_check = subparsers.add_parser(
'check',
description="Check or lint source code",
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="Check source format by clang-tidy",
+ help="Check source format by clang-tidy")
Review Comment:
done
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993249898
##########
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:
now I will only appear when clang-format in not versioned 12
##########
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:
now it will only appear when clang-format in not versioned 12
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993322856
##########
x.py:
##########
@@ -256,17 +288,25 @@ def test_go(dir: str, rest: List[str]) -> None:
subparsers = parser.add_subparsers()
+ parser_format = subparsers.add_parser(
+ 'format',
+ description="Format source code",
+ help="Format source code")
+ parser_format.set_defaults(func=lambda **args: check_format(**args, fix=True))
+ parser_format.add_argument('--clang-format-path', default='clang-format', help="path of clang-format used to check source")
+
parser_check = subparsers.add_parser(
'check',
description="Check or lint source code",
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="Check source format by clang-tidy",
+ help="Check source format by clang-tidy")
Review Comment:
I think `clang-tidy` here should be `clang-format`
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993424287
##########
.clang-format:
##########
@@ -0,0 +1,200 @@
+---
+Language: Cpp
+# BasedOnStyle: Google
Review Comment:
The old limit is 120, so I just inherit it. I think maybe 200 is not friendly to my small 1080p laptop 🤣 , but I am open for it. What do other guys think about it? cc @git-hulk @torwig @caipengbo @ShooterIT
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#issuecomment-1275976125
> @PragmaTwice Can you help to rebase those commits, I don't know which commit should I start looking.
You can just review three file: `x.py`, `.github/workflows/kvrocks.yml` and `.clang-format`, all of other file changes are made by clang-format.
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993316472
##########
.clang-format:
##########
@@ -0,0 +1,200 @@
+---
+Language: Cpp
+# BasedOnStyle: Google
Review Comment:
I'd prefer `ColumnLimit` to 200...Nowadays display device is far more wider than 80 or 120 already, and wrap function calls or string literals are really clumsy.
##########
.clang-format:
##########
@@ -0,0 +1,200 @@
+---
+Language: Cpp
+# BasedOnStyle: Google
Review Comment:
I'd prefer `ColumnLimit` to 200...Nowadays display device is far wider than 80 or 120 already, and wrap function calls or string literals are really clumsy.
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#issuecomment-1275987490
> @PragmaTwice You can squash a bunch of fix commits so they won't be merged in the unstable branch in the current representation :)
@torwig I think we're using squash and merge here :)
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993311374
##########
.clang-format:
##########
@@ -0,0 +1,200 @@
+---
+Language: Cpp
+# BasedOnStyle: Google
Review Comment:
1. `cd` to a non-project directory
2. `clang-format-12 -style=google -dump-config > .clang-format`
3. change `ColumnLimit` to 120
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#issuecomment-1275958803
@PragmaTwice Can you help to rebase those commits, I don't know which commit should I start looking.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993249279
##########
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:
done, it is `./x.py format` now.
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#issuecomment-1275909985
> @PragmaTwice Good job!
I also like this change, cpplint can't auto fix is too frustrate.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993219895
##########
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:
oh, thanks, I will delete it.
--
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
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993449951
##########
.clang-format:
##########
@@ -0,0 +1,200 @@
+---
+Language: Cpp
+# BasedOnStyle: Google
Review Comment:
I think 120 is a good decision. Of course, we can use even 200, but 120 makes developers think if the length of the line is more than 120 characters.
--
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
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993451799
##########
.clang-format:
##########
@@ -0,0 +1,200 @@
+---
+Language: Cpp
+# BasedOnStyle: Google
Review Comment:
I also think 120 is enough.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993252708
##########
.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:
I will add a summary, but I think it is useful for some circumstances. Developers do not need a clang-format 12 (code formatted by other version of clang-format may not pass the CI) since we will generate patches, which is friendly for first-time contributors.
--
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
[GitHub] [incubator-kvrocks] tisonkun merged pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#discussion_r993312952
##########
x.py:
##########
@@ -116,14 +119,43 @@ 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, fix: bool = False) -> None:
+ command = find_command(clang_format_path, msg="clang-format is required")
+
+ version_res = run_pipe(command, '--version').read().strip()
+ version_str = re.search(r'version\s+((?:\w|\.)+)', version_res).group(1)
+
+ version = check_version(version_str, CLANG_FORMAT_REQUIRED_VERSION, "clang-format")
+
+ if version[0] > 12:
+ raise RuntimeError("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:
```suggestion
raise RuntimeError("We use clang-format 12 in CI,"
" so we recommend that you also use this version locally to avoid inconsistencies."
" You can install it from your package manager (usually in clang-12 package)"
" or download it from https://github.com/llvm/llvm-project/releases/tag/llvmorg-12.0.1")
```
I suggest not to manually wrap lines.
##########
x.py:
##########
@@ -116,14 +119,43 @@ 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, fix: bool = False) -> None:
Review Comment:
```suggestion
def clang_format(clang_format_path: str, fix: bool = False) -> None:
```
... also its references.
--
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
[GitHub] [incubator-kvrocks] torwig commented on pull request #979: Use clang-format instead of cpplint
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #979:
URL: https://github.com/apache/incubator-kvrocks/pull/979#issuecomment-1275980326
@PragmaTwice You can squash a bunch of `fix` commits so they won't be merged in the `unstable` branch in the current representation :)
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #979: Use clang-format instead of cpplint
Posted by 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