-
Notifications
You must be signed in to change notification settings - Fork 123
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
Pull Request for adding scope keywords feature in rosidl_target_interfaces (Issue#400) #798
base: rolling
Are you sure you want to change the base?
Conversation
Do I need to sign-off my commit? |
Yes. Besides that, please remove other changes to this PR that don't directly have to do with the issue you are trying to fix. It makes it difficult to review and see what is actually being changed. |
@clalancette Thanks for the feedback. For the sign-off, should I rebase my branch or is it unsafe? |
Yes, rebasing is fine.
Yes. You'll need to make sure your change is "clean" on top of those, so we only see the differences you are trying to make. When you rebase to add the sign-off, you should also make sure this is the case. |
d4d5add
to
c870a90
Compare
…target_interfaces function. The feature is added for the better compatibility with the target_link_libraries command as requested in issue#400 Signed-off-by: Fahad <fahad.raza@desaysv.com>
@clalancette I have removed the other changes. Can you please review. Thanks. |
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 with green CI.
Note, can we add scope keywords are optionally supported in the doc section?
Can you be more specific, sorry I did not get it. |
@clalancette and @fujitatomoya any update? |
im sorry, bad memory 😓 I think what i mean was to add an explanation in function doc header, line 22 - |
Signed-off-by: Fahad <fahad.raza@desaysv.com>
@fujitatomoya thank you for your explanation. I have added a few comments in the doc header as you suggested. Please have a review. |
@fujitatomoya just a reminder that I have updated the doc header. |
@clalancette Hi, it's been a month since I updated the PR. I believe @fujitatomoya is busy so can someone else review and approve it please? |
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.
sorry for being late to get back.
lgtm, i would like to have another review.
This PR is for the issue#400 "Using rosidl_target_interfaces incompatible with keyword target_link_libraries command". As rosidl_target_interfaces did not have the scope keyword options it could become incompatible with the target_link_libraries command if used with a scope keyword.