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

Backport #1492 to v0.12 #1495

Merged
merged 5 commits into from
Mar 9, 2017
Merged

Conversation

cosmo0920
Copy link
Contributor

No description provided.

@@ -549,6 +553,26 @@ def configure(conf)
super

@regexp = @with_priority ? REGEXP_WITH_PRI : REGEXP
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this line?

end

def parse_auto(text, &block)
if @message_format == :auto
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this condition because parse_auto is not called directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I've overlooked this. 😖

@repeatedly
Copy link
Member

Commented. I will merged this patch after applied reviews.

`@message_format` is always :auto in #parse_auto.
class << self
alias_method :parse, :parse_plain
end
REGEXP_RFC5424
Copy link
Member

@repeatedly repeatedly Mar 9, 2017

Choose a reason for hiding this comment

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

Assigning @time_format = @rfc5424_time_format is needed when time_format is not set by user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Your suggestion means that we should set @time_format in the following situation?

@time_format = @rfc5424_time_format unless @time_format

It is reasonable for me.

Copy link
Member

@repeatedly repeatedly Mar 9, 2017

Choose a reason for hiding this comment

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

Yes, but time_format has default value for now.
So there are two approaches:

  • Change time_format's default to nil and set each default time_format in message_format condition
  • Use conf.has_key?('time_format')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to choose conf.has_key?('time_format') approach.

@repeatedly
Copy link
Member

I re-checked the code and I have one question. Commented it.

@repeatedly
Copy link
Member

Thanks!
Could you add test for only message_format rfc5424 configuration?

@cosmo0920
Copy link
Contributor Author

Could you add test for only message_format rfc5424 configuration?

I've added in 223d249.

@repeatedly repeatedly merged commit 2481157 into fluent:v0.12 Mar 9, 2017
@repeatedly
Copy link
Member

Thanks!

@cosmo0920 cosmo0920 deleted the backport-#1492-v0.12 branch March 9, 2017 09:20
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