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

[RFC] Add ability to symbolize keys so it can be used with keyword args #275

Merged
merged 1 commit into from Apr 6, 2022

Conversation

paniko0
Copy link

@paniko0 paniko0 commented Mar 3, 2020

Fixes #274

I am not sure if this is the way the maintainer wants to move on with the gem, but one approach would be adding a new argument for retro compatibility purposes and convert the argument if the argument is ready.

Essentially, I created this argument called symbolize_args and in case it was set, we'd symbolize the hash keys so before we actually send them to ActiveJob.

Can I get some comments here if this is the way to move forward? I'll implement tests once confirmed.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 91.133% when pulling 2bb4918 on paniko0:master into 25d7bc7 on ondrejbartas:master.

@ondrejbartas
Copy link
Collaborator

@paniko0 Hi arguments that sidekiq cron is loading from config are just passed to .perform_async so basically these arguments still get JSONified and send to redis. So this will not help.

This is just my opinion. Maybe I am completely wrong. Please add some test so we can verify what it actually does

@simmerz
Copy link

simmerz commented Mar 1, 2022

@ondrejbartas I don't know about the direct Sidekiq approach, but we're using ActiveJob, and so the args passed there would be sent directly to the perform method of the class.

I'd be inclined to either force all ActiveJob hash argument lists be provided as keyword parameters to the perform_later method, or allow it to be configurable somehow - either on a per job basis, but more likely at the global level.

I'd welcome your thoughts on this, but it does seem like it's needed - I'm running into the same problem too, and I'd rather have keyword params rather than a list of args in my method signature.

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.

Ability to use ActiveJobs with keyword arguments
4 participants