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 filter for custom processors #116

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Shelob9
Copy link
Contributor

@Shelob9 Shelob9 commented Feb 17, 2017

#115

I'm working on my plugin that uses that, will share once I'm more sure it works right and it's pushed to Github so you can see the use case.

@JasonHoffmann
Copy link
Contributor

JasonHoffmann commented Feb 17, 2017

This is a great extension. A couple of questions though.

  • Right now, that filter would only allow adding a single custom processor. Would it be more useful to allow for an array?
  • Would it be possible to add a test for the filter? Just checking that the filter is able to register an external class. If not, I can add one
  • There are a few code style issues (https://travis-ci.org/reaktivstudios/locomotive/jobs/202697661), but these don't need to addressed until everything is set

Other than that, I'd say this would be a welcome addition

@Shelob9
Copy link
Contributor Author

Shelob9 commented Feb 17, 2017

  • Right now, that filter would only allow adding a single custom processor. Would it be more useful to allow for an array?
    Not sure I follow the concern, this allows any number of custom processors. I updated the readme with an example that adds two custom processors.

  • Would it be possible to add a test for the filter? Just checking that the filter is able to register an external class. If not, I can add one

Yes, but I'm kind of out of time on this experiment...

@Faison
Copy link

Faison commented Feb 17, 2017

Hi @Shelob9 ,

I noticed that the original plugin code had a bit of repeat code in it and that's something I'm hoping we can start to reduce. So I've made an attempt at this and pushed it up in the branch feature-allow-custom-batch-processors, while also including your filter to allow custom batch processors.

It uses the same filter name, but instead of returning a class name string, you would return your batch processor object already instantiated. Could you checkout that branch and let me know if that change would satisfy your use case?

Thanks,
Faison

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.

3 participants