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

Enable chunk.each only for limited plugins using msgpack #1263

Merged
merged 3 commits into from
Oct 7, 2016

Conversation

tagomoris
Copy link
Member

chunk.each (chunk.msgpack_each) should be available only for chunks contains msgpack binary data.

It's original design, but not implemented when v0.14 output plugins were introduced (missed to remove include ChunkMessagePackEventStreamer from chunk.rb).
Currently there are not so many output plugins using v0.14 buffered output API, so it's time to change.

chunk.each (chunk.msgpack_each) will be available only for:

  • output plugins using standard format (doesn't implement #format method)
  • output plugins marked to have #format method returning msgpack binary data

…ontains msgpack binary data.

It's original design, but not implemented when v0.14 output plugins were introduced.
Currently there are not so many output plugins using v0.14 buffered output API, so it's time to change.
chunk.each (chunk.msgpack_each) will be available only for:
* output plugins using standard format (doesn't implement #format method)
* output plugins marked to have #format method returning msgpack binary data
…plugin returns msgpack binary

to enable chunk.each to iterate content objects.
@tagomoris
Copy link
Member Author

@repeatedly Could you review this change? I'll merge this before releasing v0.14.7.

class DummyCustomFormatBufferedOutput < DummyBareOutput
def initialize
super
@format_type_is_msgpack = nil
Copy link
Member

Choose a reason for hiding this comment

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

Who set this callback?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, I missed to add tests using custom formats.

@repeatedly
Copy link
Member

Code itself is no problem.

@tagomoris
Copy link
Member Author

I've just pushed a commit to add tests about custom format plugin.
I'll merge this after CI green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants