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.

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

add default_config verb to create a default configuration file #45

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

ottojo
Copy link
Contributor

@ottojo ottojo commented Sep 27, 2022

Information about the config file format is hard to find, i think this makes it easier to use.

This adds a verb default_config which just writes the default config file already present in the code to a file, to be customized by the package author.

Copy link

@mikeferguson mikeferguson left a comment

Choose a reason for hiding this comment

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

I just tried this out (rebased on top of main) and I do think it would be a helpful addition to this tool (everything worked as advertised)

Copy link
Collaborator

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

@ottojo thanks for the contribution and thank you @mikeferguson for reviewing this PR.

Left a couple of nitpicks to address before we can merge.

Alternatively @mikeferguson if you could open a PR with the main rebase and the copyright headers added, happy to merge that in as long as the origin commit authored by @ottojo is preserved.

@@ -0,0 +1,16 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add copyright header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a header, not sure if it is expected to dedicate copyright to OSRF, that can be done if desired.

@@ -0,0 +1,31 @@
import os
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add copyright header.

@ottojo
Copy link
Contributor Author

ottojo commented Feb 6, 2024

Nice to see some activity here, I think package documentation is such an important aspect of ROS with much room for improvement!

I rebased the commit to latest main, and tested it myself as well.

@ottojo ottojo requested a review from Yadunund February 6, 2024 20:00
Copy link
Member

@tfoote tfoote 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 pushing this forward

@Yadunund
Copy link
Collaborator

Yadunund commented Feb 7, 2024

Looks like our linters have flagged some formatting issues with the changes proposed. Do you mind addressing them?

@ottojo
Copy link
Contributor Author

ottojo commented Feb 7, 2024

should be fixed! @Yadunund

@Yadunund Yadunund merged commit 2566910 into ros-infrastructure:main Feb 7, 2024
2 checks passed
@ottojo ottojo deleted the verb_default_config branch February 7, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants