-
Notifications
You must be signed in to change notification settings - Fork 55
Enable the use of a default for parameters #84
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
Conversation
|
Thanks! This looks good - good you add default parameter to the documentation comment before each function? |
|
Oops typo mean to say "could you add default parameter to the documentation comment before each function?" |
|
Hmmm... the tests didn't report results for some reason.... |
|
@ros-pull-request-builder retest this please |
|
I'm going to close and reopen this to see if it triggers CI |
|
I'm at a bit of a loss as to why the results of the checks aren't showing up for this PR. It looks like they are running (successfully, yay!) on the build farm, just not getting reported. The webhook seems setup properly for this repo, and it has test_pull_requests set to true in rosdistro: https://github.com/ros/rosdistro/blob/7ad64364be0088e1a0dd8a9020872fff6b8ae78d/humble/distribution.yaml#L2594 @tfoote @clalancette any ideas? |
jonbinney
left a comment
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.
LGTM. I do want to get the CI sorted before merging though.
|
Sure! No worries, I am not on a tight schedule :) |
|
I've switched over to github actions for running the tests, and they actually run now! Looks like uncrustify and cpplint are unhappy - looks like the line with the function signature is too long: https://github.com/ros/filters/actions/runs/14162352725/job/39669757509#step:6:445 |
|
Fixed the linting tests. Not sure how I trigger test pipeline again |
|
Normally the actions trigger automatically when you push a commit, but for new committers github requires me to manually approve for security reasons. Tests pass! Thank you for this! |
The default value as input enables choosing your own defaults for parameters in classes inheriting from filterBase.
For the discussion leading to this PR, see this PR on laser_filters.