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

Replace "class" in Verk.Job struct #104

Closed
mkaszubowski opened this issue Jan 16, 2017 · 5 comments
Closed

Replace "class" in Verk.Job struct #104

mkaszubowski opened this issue Jan 16, 2017 · 5 comments

Comments

@mkaszubowski
Copy link

Because class sounds a bit object oriented, I guess it would be nice if it were replaced. I was thinking that module or worker would be a bit better.

Let me know what do you think, maybe I can send a pull request with the changes.

@edgurgel
Copy link
Owner

One very important feature that Verk has is the compatibility with Sidekiq/Reque clients. They use the term class and that's why we follow the same convention. We could potentially convert through Verk.enqueue, Verk.schedule so that our client will talk about "modules" but the underlying implementation should stay as class

@mkaszubowski
Copy link
Author

Of course, that makes perfect sense not to change the implementation. Having this in mind, I still think that using some other word in the public API would be a nicer and more idiomatic way of using the library.

If you're fine with this, I could try to make a PR with the changes soon.

@edgurgel
Copy link
Owner

yeah definitely would like to accept it from our client 👍

We would still need to accept class or module. Thanks for starting this!

@keyan
Copy link
Contributor

keyan commented Feb 9, 2017

Late to the discussion on this but I'm not in support of a change to the documentation on this. Possibly open to it with convincing documentation that the naming is causing confusion. But as it stands we don't really benefit from adding an additional option for job enqueueing.

The downside is that we have to do additional work to support a new enqueuing option with the same functionality and we will have to still mention the class term because not including it in the documentation would be confusing to those who plan to enqueue jobs from resque.

@edgurgel
Copy link
Owner

I'm closing this as there's no agreement and no real gain to change this.

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

No branches or pull requests

3 participants