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

Deprecate perform/1 in favor of perform/2 #45

Closed
chrismccord opened this issue Aug 3, 2019 · 1 comment

Comments

@chrismccord
Copy link

commented Aug 3, 2019

Thanks for this fantastic library!

Currently, users expect the job args to be passed to perform/1, and when they need access to the %Job{}, they define a new perform/1 clause which matches on the job. This works because Oban injects:

      def perform(%Job{args: args}), do: perform(args)
      def perform(args) when is_map(args), do: :ok

While this works with very little code injection, it's non obvious to users how to reach the Job struct when needed, and non obvious that once they define their own clause that matches the struct, they may invalidate other previously valid perform/1 clauses depending on what they are doing. Instead of the argument type dance, I propose we introduce perform/2 which receives the job struct, followed by arguments. For the general use case, this still looks quite nice:

def perform(_job, %{some: arg}) do

This way it's clear what is called and when, and also makes the job metadata instantly accessible once needed. I think the throwaway arg is a very reasonable tradeoff for the times the job info isn't needed. Thoughts?

@sorentwo

This comment has been minimized.

Copy link
Owner

commented Aug 4, 2019

As discussed on Slack, overall I'm in favor of the change. It makes the code more obvious and passing both the job and the args allows for clean pattern matching in the function head.

The only thing I'd change is the order of the arguments. In a Phoenix controller action the conn is the first argument because it is required, it must be used every time the action is called. In Oban the job argument isn't used very often, not nearly as often as args are. I would flip the order of the arguments:

def perform(%{"some" => arg}, _job)

While we are pre-1.0 there are a number of people using Oban in production and I'd like to provide a smooth upgrade experience. To that end I think perform/2 should be a required callback, perform/1 will be removed from the callbacks, and no default perform/2 function clause will be generated. That should be enough to get compile time behaviour warnings raised.

sorentwo added a commit that referenced this issue Aug 4, 2019

Remove perform/1 callback in favor of perform/2
The new `perform/2` function receives the job's args, followed by the
complete job struct. This new function signature makes it clear that the
args are always available, and the job struct is also there when it is
needed.

This is a breaking change that will require all worker modules to be
updated.

Closes #45

@sorentwo sorentwo closed this in #46 Aug 5, 2019

sorentwo added a commit that referenced this issue Aug 5, 2019

Remove perform/1 callback in favor of perform/2 (#46)
The new `perform/2` function receives the job's args, followed by the
complete job struct. This new function signature makes it clear that the
args are always available, and the job struct is also there when it is
needed.

This is a breaking change that will require all worker modules to be
updated.

Closes #45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.