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 manually triggered mode for zero plugin #2329

Merged
merged 14 commits into from
Dec 26, 2016
Merged

Add manually triggered mode for zero plugin #2329

merged 14 commits into from
Dec 26, 2016

Conversation

SJoshBrown
Copy link
Contributor

I'm fairly new to the process so I hope I've done this correctly. Please let me know if I need to make any changes to get this merged up.

This is for Issue #2274

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fantastic! Thank you for adding tests and amending the docs too.

Since it looks like some code was duplicated between the new process_item method and the old write_event, could you please factor out the common code so we only have one copy?

@SJoshBrown
Copy link
Contributor Author

SJoshBrown commented Dec 18, 2016

I've taken another pass at this and factored out some of the shared code. I also added a few more tests to increase the coverage.

#2274

@nathdwek
Copy link
Member

nathdwek commented Dec 24, 2016

I'm interested in this feature being added for consistency's sake, so I took a look at the whole plugin.
There is only one important thing I think should be added before going through with this, that is adding safeguards in manual mode when the query is empty, because of the nature of the operation (see #1286 )

However, I think the original plugin is a mess in terms of style and structure, cf my review. Maybe this is the occasion to revisit that?

E: Ok so it turns out you cannot review untouched code. I'll do a separate PR for that.

@sampsyo
Copy link
Member

sampsyo commented Dec 24, 2016

Indeed, the plugin as it was was a bit of a mess. Thanks for looking into it, @nathdwek!

@nathdwek
Copy link
Member

I opened #2345 . Some unexpected issues in the testing department, but would you be able to rewrite on top of that when that's fixed? We can also do it the other way around if it's more convenient for you?

@SJoshBrown
Copy link
Contributor Author

I'm happy to wait until it gets some cleaning up. Then I can come back and rewrite this

@nathdwek
Copy link
Member

I'll see tomorrow if I can sort this test thing out with a fresh mind. But my absolute priority is not getting in your way!

@sampsyo
Copy link
Member

sampsyo commented Dec 25, 2016

Either way seems to work from my perspective—I'd also be happy to merge this new feature first and then do the refactoring afterward.

@SJoshBrown
Copy link
Contributor Author

@sampsyo @nathdwek, I think this should be ready to go now. Let me know if there are any other changes we should make

@sampsyo sampsyo merged commit b6577b4 into beetbox:master Dec 26, 2016
sampsyo added a commit that referenced this pull request Dec 26, 2016
Add manually triggered mode for zero plugin
sampsyo added a commit that referenced this pull request Dec 26, 2016
@sampsyo
Copy link
Member

sampsyo commented Dec 26, 2016

All merged up. ✨ Thank you for taking care of this! It looks perfect.

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