-
Notifications
You must be signed in to change notification settings - Fork 95
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
Upgrade cpplint and fix new errors #680
Conversation
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Codecov Report
@@ Coverage Diff @@
## sdf9 #680 +/- ##
==========================================
- Coverage 86.83% 86.83% -0.01%
==========================================
Files 62 62
Lines 9734 9731 -3
==========================================
- Hits 8453 8450 -3
Misses 1281 1281
Continue to review full report at Codecov.
|
sdf::filesystem::append(PROJECT_SOURCE_PATH, "sdf", "1.6", "1_5.convert"); | ||
const std::string CONVERT_DOC_16_17 = | ||
sdf::filesystem::append(PROJECT_SOURCE_PATH, "sdf", "1.7", "1_6.convert"); | ||
static std::string ConvertDoc_15_16() |
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.
So did cpplint actually catch this?
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.
I meant to mention this in the description, but yes, cpplint caught the global string as a runtime/string
error:
https://github.com/azeey/sdformat/blob/f5c8bb383fe13d2417f960afb261d626f1cbd40c/tools/cpplint.py#L5455. This helps some with the GSG violations we talked about, but it only works for strings.
we depend on ignition-math6, which depends on ignition-cmake2, so I consider it a valid dependency. It would only be used as a build dependency anyway |
Okay, I knew about the dependency, but I wasn't sure if it would be possible to build |
cmake fails to find
so I think it's safe to add an explicit dependency |
Sounds good. I've created a follow up PR #682 |
Summary
Upgrade cpplint to the version found in
ign-cmake2
. @scpeters and I had discussed using ign-cmake to create acodecheck
target, but it looks like we don't directly depend onign-cmake
in thesdf9
branch. So instead, I simply copiedcpplint.py
fromign-cmake
. We might be able to add thecodecheck
target forsdf10
Test it
sh tools/code_check.sh
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge