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

fix for configuration_option ArgumentError with ruby3 #387

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vbargsten
Copy link
Contributor

Unsure if this is the intended way, but it works for me on Ubuntu 22.04. Problem is described here: https://rubyreferences.github.io/rubychanges/3.0.html#keyword-arguments-are-now-fully-separated-from-positional-arguments, i.e. number of arguments wrong, 3 given expected 2.

Copy link
Member

@doudou doudou left a comment

Choose a reason for hiding this comment

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

You are hitting one of these difficult backward compatible changes in some cases but not in others ...

You are calling Autoproj.configuration_option passing a hash as third argument, which is why you only need the ** in config.declare. Other people that call configuration_option with keyword syntax will need the ** there as well, but that might break your use case.

Could you try it out ? Adding ** in front of options line 33 ?

My apologies for the very late reply. Hectic year.

@vbargsten
Copy link
Contributor Author

That looks indeed more consistent. I don't see a difference. However, it could break callers using a hash as argument (and not **hash transforming it into keyword arguments).

@pierrewillenbrockdfki
Copy link
Contributor

@doudou does this look right? CI seems to be failing due to bundler now hard requiring ruby 3. This seems to work for autoproj status on ubuntu 22.04/ruby 3, ubuntu 20.04/ruby 2.7, ubuntu 18.04/ruby 2.5

@chhtz
Copy link

chhtz commented Mar 11, 2024

Wouldn't replacing Autoproj.configuration_option by Autoproj.config.declare everywhere also resolve this issue? (I made some PRs for 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.

4 participants