-
Notifications
You must be signed in to change notification settings - Fork 34
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
Adding CONFIG option #211
Adding CONFIG option #211
Conversation
fbd772a
to
5f55e2b
Compare
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.
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.
The PR looks mostly fine to me. It would be great if we can test it in combination with ign-physics following the initial use case.
Before merging, I would appreciate to have a test in place for this new feature.
356d211
to
5d27da7
Compare
Hey @j-rivero , what kind of tests would be required, could you guide me with some previous examples? |
Sorry for not providing some pointers. I see at least two alternatives:
|
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.
@j-rivero I opened a PR in ign-physics
that makes use of this PR if you want to take a look: gazebosim/gz-physics#339
@harshmahesheka, other than the testing comments mentioned above, I left one last minor comment if you wouldn't mind addressing it before we merge this.
Hey @adlarkin, sorry for missing it. I have merged the suggested changes. |
e3d98eb
to
2a328f4
Compare
Signed-off-by: Harsh Mahesheka <harsh.mahesheka.eee20@iitbhu.ac.in>
Typos corrections Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org> Signed-off-by: Harsh Mahesheka <harsh.mahesheka.eee20@iitbhu.ac.in>
Adding space between [CONFIG] and [BUILD_ONLY] Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org> Signed-off-by: Harsh Mahesheka <harsh.mahesheka.eee20@iitbhu.ac.in>
Adding space Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org> Signed-off-by: Harsh Mahesheka <harsh.mahesheka.eee20@iitbhu.ac.in>
2a328f4
to
88bc149
Compare
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.
@harshmahesheka we are going to merge this PR now since a few other PRs depend on it, but if you still want to add tests, feel free to do that in a separate PR. Thanks for the contribution!
Signed-off-by: Harsh Mahesheka <harsh.mahesheka.eee20@iitbhu.ac.in> Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org>
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-releases-2022-04-27-fortress-citadel/1389/1 |
This is a PR with regards to the issue #114 .