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

use Event#from_json if available #21

Merged

Conversation

colinsurprenant
Copy link
Contributor

relates to https://github.com/elastic/logstash/pull/4631and elastic/logstash#4595

similar PR as logstash-plugins/logstash-codec-json_lines/pull/19 to support the new Event#from_json if available when using the Java Event.

@colinsurprenant colinsurprenant changed the title support for Event#from_json use Event#from_json if available Feb 8, 2016
@talevy
Copy link
Contributor

talevy commented Feb 8, 2016

looks good, have the same concern about testing as mentioned here: logstash-plugins/logstash-codec-json_lines#19 (comment)

@colinsurprenant
Copy link
Contributor Author

@talevy I used the same strategy as json_lines, let me know if all good.


def from_json_parse(json, &block)
LogStash::Event.from_json(json).each { |event| yield event }
rescue LogStash::Json::ParserError => e
Copy link
Contributor

Choose a reason for hiding this comment

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

why no begin block? I prefer the indentation that we get by using the begin, but I also understand it is a bit redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, its a style preference, I actually like no having the extra indentation of the begin! 😉

@talevy
Copy link
Contributor

talevy commented Feb 9, 2016

small comment, ott LGTM

@colinsurprenant
Copy link
Contributor Author

thanks, squashing, bumping and merging.

run specs for both Event#from_json or legacy context if possible
@colinsurprenant colinsurprenant merged commit 52baa87 into logstash-plugins:master Feb 9, 2016
@colinsurprenant
Copy link
Contributor Author

published 2.1.0

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.

2 participants