Run various linters on pull requests #53
Conversation
|
It is fairly easy to reformat C++ source files as well with clang-format, but we need to decide on some coding style guidelines. A comparison between most used guidelines can be found at https://github.com/motine/cppstylelineup, please tell which one yu want. |
|
@fteicht or @jeromerobert can you comment and contribute on which c++ style to apply on C++ files |
|
Thanks @dbarbier for digging int the topic. Personally I am used to the LLVM style that I like a lot. The Google style is fine to me too. What do you think @jeromerobert ? |
|
LLVM is also my prefered one. I used Webkit for a while but 2 spaces indentation is definitely better. Google is good except when it create one line |
|
Okay I just added some commits to call clang-format with LLVM style on all C++ files in cpp/src directory. As mentioned in 545c3c7 I had to disable sort of includes, otherwise there are build failures. |
If linters report errors, sources have to be modified
before being merged. Since builds take time on runners,
abort early to not waste CPU cycles.
Add .clang-format configuration file generated by
clang-format-9 --dump-config --style=LLVM > .clang-format
clang-format-9 --dump-config --style='{SortIncludes: false}' > new-clang-format && mv new-clang-format .clang-format
Github's ubuntu-latest runners have clang-format versions
between 6 and 10, but there is no clang-format 10 on Debian,
and thus use version 9.
We have to set SortIncludes to false, otherwise there are build
failures because files in /impl/ subdirectories may be #include'd
too early. This problem should be fixed, but for now do not sort
includes.
|
Force-pushed to fix conflicts |
Black will be applied on examples/*.py and modify these delimiters.
|
Black is now run on docs and examples too, and data files are no more processed. This PR is no more a draft and can be reviewed. |
|
Since there are many open pull requests, I dropped commits which were applying linters in order to avoid conflicts. |
Ok but how can I review it, then? Which script do I need to run on my side? |
|
This can be reactivated when all files are fixed. At the moment, files are not modified to avoid conflicts with other PRs.
|
Might also be interesting to have a pre-commit hook for |
|
It is IMO more important to have this PR merged and source files modified without messing with other PRs. |
|
The C++ linter does not seem to cut long lines. Is there a way to configure it so it can cut the lines appropriately like |
1 similar comment
|
The C++ linter does not seem to cut long lines. Is there a way to configure it so it can cut the lines appropriately like |
Brew does not provide clang-format v9, so MacOS users may not be able to easily run pre-commit locally. We may try with v8, but since it is unclear whether we really need a versioned dependency on clang-format, let us try with an unversioned one for now, and we will get back to a versioned dependency if discrepancies are found.
|
Okay the problem was that clang-format-9 was not found on Mac. Brew does not provide this version, so I removed the versioned dependency. Output is the same as before, hopefully we do not need to pin a specific version. |
|
The issue was that |
This PR is quite self-explanatory, it adds a new "Code style" check at the bottom of this page in the checks tab.
Mark this PR as a draft for now, we have to discuss which linters do we want to apply and on which files, eg. do we want to reformat documentation and notebooks too? Also this PR should be applied only when there is no conflict with other PRs.
Checks will be performed on PRs created after this PR is merged, and if some check fails, author will have to fix source files.