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

Suppress warning about keyword argument introduced from ruby 2.7 #2664

Merged
merged 1 commit into from
Oct 28, 2019

Conversation

ganmacs
Copy link
Member

@ganmacs ganmacs commented Oct 24, 2019

Which issue(s) this PR fixes:
no

What this PR does / why we need it:

For https://bugs.ruby-lang.org/issues/14183

there are a vast number of warnings in https://travis-ci.org/fluent/fluentd/jobs/602120485 which is tested in ruby 2.7 preview2.

See also https://www.ruby-lang.org/en/news/2019/10/22/ruby-2-7-0-preview2-released/
Docs Changes:

no need

Release Note:

no need

@ganmacs ganmacs self-assigned this Oct 24, 2019
@ganmacs ganmacs force-pushed the for-ruby2.7 branch 2 times, most recently from 01f3dc8 to d6dab35 Compare October 24, 2019 06:25
@ganmacs
Copy link
Member Author

ganmacs commented Oct 24, 2019

Warnings around keyword arguments look disappearing https://travis-ci.org/fluent/fluentd/jobs/602174792?utm_medium=notification&utm_source=github_status

@ganmacs ganmacs marked this pull request as ready for review October 24, 2019 07:14
@ganmacs ganmacs requested a review from repeatedly October 24, 2019 07:14
@@ -53,13 +53,26 @@ def initialize(klass, opts: {}, &block)
@section_name = ''
end

def configure(conf, syntax: :v1)
def configure(conf)
Copy link
Member

Choose a reason for hiding this comment

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

this change doesn't impact existing test cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

looks like so. this was introduced in #1047 . but I can't get any more information about why this method needs this parameter. so leave it as it is. and I will delete it when fluentd becomes v2. or you think it should be deleted in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, this parameter is for testing the plugin with v1 and old syntax.
I'm not sure how many 3rd party plugins use this parameter...

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no problem for me because this is only for test, not for production code.
Failing test is not critical and easy to fix it.

@ganmacs ganmacs merged commit d9533da into fluent:master Oct 28, 2019
@ganmacs ganmacs deleted the for-ruby2.7 branch October 28, 2019 06:26
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