-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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] fix CMakeLint errors related to function naming case #4794
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are false alerts for readability/wonkycas in some built-in CMake
@StrikerRUS I don't think we need to filter out the readability/wonkycase
warnings.
I think the mixed-case commands you mentioned can be lower-cased. I tried that tonight and was able to run the tests successfully.
# lowercase commands
sed -i.bak -e "s/FetchContent_Declare/fetchcontent_declare/" CMakeLists.txt
sed -i.bak -e "s/FetchContent_MakeAvailable/fetchcontent_makeavailable/" CMakeLists.txt
# run tests
rm -rf build/
mkdir build/
cmake -DBUILD_CPP_TEST=ON -DUSE_OPENMP=OFF -DUSE_DEBUG=ON ..
make testlightgbm -j4
./../testlightgbm
That's consistent with https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#command-invocations, which says
Command names are case-insensitive
Hmmm, thanks for clarifying that those commands can be lower-cased. But in the official docs they are mixed-cased. Don't you think we should stick to the official (which I personally consider as a recommended) CMake naming style but not to |
Personally, I'd prefer to use the lower-cased one so Otherwise, we'll have to write comments like "can you please change |
These style issues will produce |
ohhhhh I understand, yeah ok I am ok with filtering out |
Nice work! Just 3 warnings from https://github.com/microsoft/LightGBM/runs/4201144410?check_suite_focus=true |
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Contributes to #4602.
Use lower case for all functions to fix all
readability/mixedcase
errors.There are false alerts for
readability/wonkycas
in some built-in CMake functions, e.g.LightGBM/CMakeLists.txt
Line 501 in 6cbb358
LightGBM/CMakeLists.txt
Line 506 in 6cbb358
https://cmake.org/cmake/help/latest/module/FetchContent.html
So the option only is to not check
readability/wonkycase
type of errors. Because there is noNOLINT
directive in thecmakelint
package like it is in thecpplint
package 🙁 .