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

Mixed optional and keywords arguments in BaseTask #401

Closed
microstudi opened this issue Dec 13, 2021 · 2 comments · Fixed by #402
Closed

Mixed optional and keywords arguments in BaseTask #401

microstudi opened this issue Dec 13, 2021 · 2 comments · Fixed by #402

Comments

@microstudi
Copy link
Contributor

PR #394 introduced a new argument in BaseTask:

def initialize(config = {}, config_file: nil)

The problem is that uses a keyword argument after an optional argument. This leads to situations where backward compatibility is broken. For instance, this used to work fine:

I18n::Tasks::BaseTask.new(locales: some_locales)

Now it throws an ArgumentError because ruby tryies to allocate the passed arguments as part of the keywords hash.
So now you have to call the method like this, which seems illogical to me:

I18n::Tasks::BaseTask.new({locales: some_locales}, config_file: nil)

This is going to broke some CI tests around. Maybe it would be better to use only one style for the method argument definition?

@glebm
Copy link
Owner

glebm commented Dec 13, 2021

Yeah, makes sense, let's change it to something like this (PR welcome)

def initialize(config_file: nil, **config)

@microstudi
Copy link
Contributor Author

will do! thanks

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 a pull request may close this issue.

2 participants