Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove default_value for required arguments #602

Merged
merged 11 commits into from
Sep 11, 2024
Merged

Remove default_value for required arguments #602

merged 11 commits into from
Sep 11, 2024

Conversation

Amronos
Copy link
Contributor

@Amronos Amronos commented Sep 6, 2024 •
edited
Loading

🦟 Bug fix

Removes default_value for config_file and bridge_name due to them being a required argument.
Changes string and xml_string to world_string in various places to maintain consistency, make clarification, and make fixes.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@Amronos
Remove default_value for config_file
9bcefd2 
Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
ahcorde and others added 2 commits September 6, 2024 11:31
@ahcorde
Merge branch 'ros2' into remove-default-values
699ae29
@Amronos
Update ros_gz_sim.launch
c130c91 
Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
@caguero caguero mentioned this pull request Sep 6, 2024
Closed
8 tasks
caguero
caguero reviewed Sep 6, 2024
View reviewed changes
Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch!

Could you fix the conflicts?

caguero
caguero requested changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also see config_file instances to be updated in:

  • ros_gz_bridge/launch/ros_gz_bridge.launch
  • ros_gz_bridge/launch/ros_gz_bridge.launch.py

@Amronos
Fix errors with name of bridge not being given (gazebosim#600)
9ed5613 
* Add argument bridge_name to fix errors

Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
@Amronos
Merge branch 'ros2' into remove-default-values
cf46197
@Amronos
Remove default values for config_file and bridge_name
e9fac33 
Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
@Amronos Amronos requested a review from caguero September 7, 2024 03:17
@Amronos
Change xml_string to string
59e9fe6 
Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
@Amronos
Copy link
Contributor Author

Amronos commented Sep 8, 2024 •
edited
Loading

Changed xml_string to string in various places to maintain consistency and make fixes. Please review that too.

@Amronos
Change xml_string to string in actions
b8c55b0 
Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
@Amronos Amronos mentioned this pull request Sep 8, 2024
Open
9 tasks
@caguero
Copy link
Contributor

caguero commented Sep 9, 2024 •
edited
Loading

Changed xml_string to string in various places to maintain consistency and make fixes. Please review that too.

Naming is hard but I thought xml_string was a lightly better name than string. Maybe sdf_string is better? I don't have a strong opinion here :) . Maybe world_string?

@Amronos
Change string and xml_string to sdf_string
385d6c3 
Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
@Amronos
Change sdf_string to world_string
4998bb6 
Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
@Amronos
Copy link
Contributor Author

Amronos commented Sep 9, 2024

I think now there are no more changes to be made. Can this merged?

@Amronos
Missed one
874feff 
Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
@caguero caguero merged commit 7e0c94c into gazebosim:ros2 Sep 11, 2024
3 checks passed
@Amronos Amronos deleted the remove-default-values branch September 11, 2024 17:49
@caguero caguero mentioned this pull request Sep 11, 2024
Merged
8 tasks
Amronos added a commit to Amronos/ros_gz that referenced this pull request Sep 12, 2024
@Amronos
Remove default_value for required arguments (gazebosim#602)
e4b4871 
* Remove default_value for config_file
Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
Amronos added a commit to Amronos/ros_gz that referenced this pull request Sep 18, 2024
@Amronos
Remove default_value for required arguments (gazebosim#602)
40a2f6a 
* Remove default_value for config_file
Signed-off-by: Aarav Gupta <134804732+Amronos@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caguero caguero caguero approved these changes

@ahcorde ahcorde Awaiting requested review from ahcorde ahcorde is a code owner

Assignees
No one assigned
Labels
None yet
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Amronos @caguero @ahcorde