Navigation Menu

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 keyword arguments support in extensions #5120

Merged
merged 4 commits into from Jan 18, 2022
Merged

Add keyword arguments support in extensions #5120

merged 4 commits into from Jan 18, 2022

Conversation

4e4c52
Copy link
Contributor

@4e4c52 4e4c52 commented Jan 13, 2022

This PR allows the delay extension to support keyword arguments.
I know the extensions will be deleted in 7.0 but it would be a nice addition in the meantime.

@mperham
Copy link
Collaborator

mperham commented Jan 13, 2022

Nice, good catch.

There doesn't appear to be a test for the actual perform call, asserting that any kwargs are filled in as expected.

@4e4c52
Copy link
Contributor Author

4e4c52 commented Jan 14, 2022

@mperham I added more test to check the keyword arguments forwarding 😉

@mperham
Copy link
Collaborator

mperham commented Jan 14, 2022

Breaks on Ruby 2.6.

@4e4c52
Copy link
Contributor Author

4e4c52 commented Jan 18, 2022

I added a condition to check if the kwargs are empty so I do not pass them to the proxied object.
Looks like it has to do with what's described here: https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html

@mperham mperham merged commit 0a4de94 into sidekiq:main Jan 18, 2022
@mperham
Copy link
Collaborator

mperham commented Jan 18, 2022

Thank you!

@mperham
Copy link
Collaborator

mperham commented Jan 31, 2022

I reverted this support. It was becoming a maintenance chore; they will be removed soon and I don't have any desire to increase their maintenance burden in the meantime.

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.

None yet

2 participants