Fix ament_lint_cmake line length expression#236
Merged
Conversation
This regular expression is using the re.VERBOSE flag, meaning that characters after an un-escaped '#' character are interpreted as a comment and are not part of the expression. Signed-off-by: Scott K Logan <logans@cottsay.net>
ivanpauno
reviewed
Jan 28, 2021
I'm setting this in the CMake instead of the cmakelint source code to avoid further diverging from the upstream cmakelint code. Signed-off-by: Scott K Logan <logans@cottsay.net>
Contributor
Author
|
Trying again with the maximum set to 120 columns: EDIT: 120 columns yields 60 violations across ~22 packages |
ivanpauno
approved these changes
Jan 28, 2021
ivanpauno
left a comment
There was a problem hiding this comment.
LGTM! (when CI is green)
If the max line length is changed again to make more packages pass, that still looks good to me.
Signed-off-by: Scott K Logan <logans@cottsay.net>
Contributor
Author
|
Trying again at 140 - maybe after we get this merged, we can ratchet it back to 120 someday. |
cottsay
added a commit
to ros2/spdlog_vendor
that referenced
this pull request
Feb 11, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rmw
that referenced
this pull request
Feb 11, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rmw_implementation
that referenced
this pull request
Feb 11, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rosbag2
that referenced
this pull request
Feb 11, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rosidl
that referenced
this pull request
Feb 11, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rmw_dds_common
that referenced
this pull request
Feb 11, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rcutils
that referenced
this pull request
Feb 11, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
This was referenced Feb 11, 2021
Contributor
Author
|
Here's the CI build targeting the 9 packages with 140 column violation, on branches with the violations resolved. Once the linked PRs are in, I'll merge this one.
|
nuclearsandwich
pushed a commit
to eProsima/foonathan_memory_vendor
that referenced
this pull request
Feb 12, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rcutils
that referenced
this pull request
Feb 12, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rmw_dds_common
that referenced
this pull request
Feb 12, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rmw_implementation
that referenced
this pull request
Feb 12, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rosbag2
that referenced
this pull request
Feb 12, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
clalancette
pushed a commit
to ros-visualization/python_qt_binding
that referenced
this pull request
Feb 12, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rmw
that referenced
this pull request
Feb 12, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rosidl
that referenced
this pull request
Feb 12, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
Contributor
|
This is probably the cause of some failing tests in CI, like: https://ci.ros2.org/view/nightly/job/nightly_osx_release/1964/ . I'm guessing that we just need to fix up the cmake files in |
cottsay
added a commit
to ros2/rmw_connext
that referenced
this pull request
Feb 14, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rosidl_typesupport_connext
that referenced
this pull request
Feb 14, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
This was referenced Feb 14, 2021
cottsay
added a commit
to ros2/rosidl_typesupport_connext
that referenced
this pull request
Feb 14, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
added a commit
to ros2/rmw_connext
that referenced
this pull request
Feb 14, 2021
The line length enforcement in ament_lint_cmake has been broken for some time, but will be fixed by ament/ament_lint#236. This change brings this package into compliance with a 120 column limit. Signed-off-by: Scott K Logan <logans@cottsay.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This regular expression is using the re.VERBOSE flag, meaning that characters after an un-escaped '#' character are interpreted as a comment and are not part of the expression. From what I can tell, this has been broken ever since the code was introduced. We've essentially only been getting proper line length detection for strings in a
message()call.Unfortunately, there are currently 792 line length violations in ROS 2 that have been unnoticed because of this bug:
Personally, I believe that 80 characters may be a bit aggressive for CMake. Maybe we should try changing this limit to a larger number, fix the violations and merge this fix to prevent more regressions, and then maybe bring the limit down once the enforcement is working right. I'm open to suggestions.