-
Notifications
You must be signed in to change notification settings - Fork 101
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
Generate license using ament_copyright #105
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.
Accept, up to two inline comments.
@@ -1,3 +1,32 @@ | |||
# Copyright 2021 PickNik Inc. |
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.
It's an old discussion, with a relatively new touch in ROS2. Do we really want copyright texts in all launch files?
This clutters otherwise mostly trivial files (granted, the ROS2 files do not look that trivial at the moment...)
I would vote to leave them out there unless it's impossible to silence some automatic pedantic checker.
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.
Unfortunately, I'm not aware of a way to exclude certain files from ament_copyright linter, removing the copyright from this file will cause the linter to report failure
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.
ament_copyright
has an --exclude
option. Can't we use that?
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.
But how would we use it with ament_lint_auto_find_test_dependencies
.? we either need to remove it and explicitly list all linters' cmake test ourselves (I tried this and it will complicate the BUILD_TESTING
section) or disable it and create a pre-commit hook to handle having exclude argument
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.
But how would we use it with ament_lint_auto_find_test_dependencies .?
If the macro does not provide a way for that already, it might be worthwhile to add a cmake variable to the command (AMENT_COPYRIGHT_OPTIONS
) upstream to set the behavior in another line before the call?
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.
it might be worthwhile to add a cmake variable to the command (AMENT_COPYRIGHT_OPTIONS)
I like this idea, added to my todo list
To unblock the galactic & rolling releases