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 a lot of warnings #852

Merged
merged 1 commit into from
Mar 24, 2016
Merged

Conversation

okkez
Copy link
Contributor

@okkez okkez commented Mar 15, 2016

This is the first step to wipe out warnings.

@okkez
Copy link
Contributor Author

okkez commented Mar 15, 2016

Some warnings may conflict with other PRs, so I leave these warnings.

else
convert_type(name, value)
end
record[name] = convert_type(name, value)
Copy link
Member

Choose a reason for hiding this comment

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

Calling convert_type heavily is high cost.
So we should not call convert_type when user doesn't specify types parameter.

@okkez
Copy link
Contributor Author

okkez commented Mar 15, 2016

I have fixed and pushed.
How about this?

@tagomoris
Copy link
Member

LGTM. (Squash required after all reviewer comments.)

@sonots
Copy link
Member

sonots commented Mar 16, 2016

Is it possible to integrate ruby warning check into CI? Otherwise, I think same things happen again in the future.

@sonots
Copy link
Member

sonots commented Mar 16, 2016

Good job anyway, this allows plugin developers to use ruby -w for their own plugins. Before, fluentd itself showed lots of warnings, so ruby -w for plugins did not work effectively.

@tagomoris
Copy link
Member

@sonots what is "ruby warning check" you mentioned?

@sonots
Copy link
Member

sonots commented Mar 16, 2016

Checking whether ruby -w tells warnings or not

@tagomoris
Copy link
Member

It's impossible to remove all warnings immediately (because there are some/many warnings from libraries which fluentd depends on).
IMO, checking ruby -w outputs looks good idea at a point in future, but we cannot add it right now.

* Do not shadow local variables

test/plugin/test_in_dummy.rb:91: warning: assigned but unused variable - time
lib/fluent/plugin/in_dummy.rb:45: warning: shadowing outer local variable - e
test/test_event.rb:94: warning: shadowing outer local variable - time
test/test_event.rb:142: warning: shadowing outer local variable - time
test/plugin/test_in_stream.rb:20: warning: shadowing outer local variable - time
test/plugin/test_in_stream.rb:35: warning: shadowing outer local variable - time
test/plugin/test_in_stream.rb:51: warning: shadowing outer local variable - time
test/plugin/test_in_stream.rb:68: warning: shadowing outer local variable - time
test/plugin/test_in_stream.rb:84: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:43: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:59: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:75: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:113: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:150: warning: shadowing outer local variable - tag
test/plugin/test_in_http.rb:163: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:184: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:200: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:238: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:262: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:280: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:43: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:59: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:75: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:113: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:150: warning: shadowing outer local variable - tag
test/plugin/test_in_http.rb:163: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:184: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:200: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:238: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:262: warning: shadowing outer local variable - time
test/plugin/test_in_http.rb:280: warning: shadowing outer local variable - time

* Initialize uninitialized instance variables

lib/fluent/test/base.rb:31: warning: instance variable @now not initialized
lib/fluent/test/input_test.rb:121: warning: instance variable @run_post_conditions not initialized
lib/fluent/log.rb:324: warning: instance variable @suppress_repeated_stacktrace not initialized
lib/fluent/config/configure_proxy.rb:183: warning: instance variable @current_description not initialized
lib/fluent/plugin/buf_file.rb:147: warning: instance variable @symlink_path not initialized
lib/fluent/output.rb:212: warning: instance variable @secondary not initialized
lib/fluent/output.rb:220: warning: instance variable @secondary not initialized
lib/fluent/mixin.rb:201: warning: instance variable @localtime not initialized
lib/fluent/mixin.rb:201: warning: instance variable @Timezone not initialized
lib/fluent/mixin.rb:149: warning: instance variable @remove_tag_prefix not initialized
lib/fluent/mixin.rb:150: warning: instance variable @remove_tag_suffix not initialized
test/config/test_system_config.rb:81: warning: instance variable @emit_error_log_interval not initialized
lib/fluent/plugin/in_syslog.rb:111: warning: instance variable @use_default not initialized
lib/fluent/root_agent.rb:89: warning: instance variable @without_source not initialized
lib/fluent/process.rb:488: warning: instance variable @detach_process not initialized

* Define assert_equal_event_time once

lib/fluent/test/base.rb:35: warning: method redefined; discarding old assert_equal_event_time
lib/fluent/test/base.rb:35: warning: previous definition of assert_equal_event_time was here

* Add '_' prefix to unused variables

test/test_parser.rb:1046: warning: assigned but unused variable - time
lib/fluent/test/output_test.rb:91: warning: assigned but unused variable - record
lib/fluent/config/element.rb:138: warning: assigned but unused variable - block

* Remove unused variables

test/test_parser.rb:453: warning: assigned but unused variable - format
test/test_mixin.rb:211: warning: assigned but unused variable - d
test/test_formatter.rb:55: warning: assigned but unused variable - formatter
test/test_buffer.rb:581: warning: assigned but unused variable - e
test/config/test_dsl.rb:164: warning: assigned but unused variable - root
lib/fluent/parser.rb:619: warning: assigned but unused variable - e
lib/fluent/plugin/in_exec.rb:56: warning: assigned but unused variable - localtime
lib/fluent/plugin/in_exec.rb:58: warning: assigned but unused variable - utc
lib/fluent/event_router.rb:105: warning: assigned but unused variable - c

* Rename block parameter

lib/fluent/config/v1_parser.rb:159: warning: shadowing outer local variable - path
lib/fluent/config/parser.rb:84: warning: shadowing outer local variable - path
lib/fluent/plugin/buf_file.rb:175: warning: assigned but unused variable - chunk
lib/fluent/plugin/buf_file.rb:177: warning: assigned but unused variable - timestamp
lib/fluent/plugin/buf_file.rb:181: warning: assigned but unused variable - chunk
lib/fluent/plugin/buf_file.rb:183: warning: assigned but unused variable - timestamp

* Use defined global variable

lib/fluent/plugin.rb:129: warning: global variable `$log' not initialized

* Remove redundant condition

lib/fluent/log.rb:255: warning: instance variable @suppress_repeated_stacktrace not initialized
lib/fluent/log.rb:261: warning: instance variable @suppress_repeated_stacktrace not initialized

* Define instance method once

lib/fluent/plugin/in_tail.rb:457: warning: method redefined; discarding old swap_state
lib/fluent/plugin/in_tail.rb:457: warning: previous definition of swap_state was here
@okkez okkez force-pushed the suppress-ruby-warnings branch from 07cac37 to 76c3ac2 Compare March 24, 2016 08:53
@okkez
Copy link
Contributor Author

okkez commented Mar 24, 2016

@tagomoris squashed!

@tagomoris
Copy link
Member

@okkez Thanks! I'll merge this after Travis green (AppVeyor looks in trouble now).

@tagomoris tagomoris merged commit d2c9a78 into fluent:master Mar 24, 2016
@tagomoris
Copy link
Member

Merged. Thank you!

@okkez okkez deleted the suppress-ruby-warnings branch September 28, 2016 07:09
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.

4 participants