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

Feature/remove default arguments warning for volume extension #292

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

blooop
Copy link
Contributor

@blooop blooop commented Oct 16, 2024

when running rocker this warning appears every time:

Extension volume doesn't support default arguments. Please extend it.

This sets default arguments for the volume extension to remove this warning.

It also adds type hints for the parser argument.

@blooop blooop requested a review from tfoote as a code owner October 16, 2024 10:26
@blooop
Copy link
Contributor Author

blooop commented Oct 16, 2024

Oh sorry. I was not up to date with master where you already fixed this. I guess this pr is just adding type hints now.

Copy link
Collaborator

@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 the attention to clear the warning. Sorry we had the collision on fixing it. But adding the type annotations sounds good.

PS would you be willing to add some of your rocker extensions to this list? https://github.com/osrf/rocker?tab=readme-ov-file#externally-maintained-extensions

@tfoote tfoote merged commit 3c9ea15 into osrf:main Oct 16, 2024
4 checks passed
@blooop blooop deleted the feature/remove_default_arguments_warning branch October 16, 2024 16:54
@blooop
Copy link
Contributor Author

blooop commented Oct 16, 2024

Yes, I'm happy to add my rocker extension to the list. I was holding off because I'm still getting my head around things and didn't want to advertise them in case I needed to make any breaking changes. In fact today I was considering making a meta-package that includes all the rocker extensions but I need to think through all the tradeoffs before I go ahead with that.

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.

2 participants