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

Updates dependencies and reduce warnings #1193

Merged
merged 8 commits into from
Aug 31, 2016

Conversation

tagomoris
Copy link
Member

This change is to reduce warnings when we run rake test.
Almost all warnings are about uninitialized variables, and some others are about unused variables.

I did 2 changes at the same time:

Updating dependencies

There seems no reason to specify minor version dependency for some gems. I updated these to depends on (recent) major versions. I think this operation doesn't make any problems, because all of these are development dependencies.

I also remove json runtime dependency, to use bundled json of Ruby runtime.
Specifying json version is bad practice, because it makes problem between ruby version limitations and other module's dependencies

Fix implementation of in_monitor_agent

The previous implementation depends on the behavior of eval for string expression, NoMethodError for nil objects and rescuing all errors (and do nothing).
It hides any types of unexpected errors... very bad practice.

I rewrote that code with instance_exec with proc objects, and throw-catch for explicit skip for objects in unexpected states. It shows warnings for unexpected errors in logs.

@tagomoris
Copy link
Member Author

@repeatedly I will include this patch for v0.14.3. Could you take a glance on it?

@@ -159,7 +159,7 @@ def test_configure
d = create_driver("
@type monitor_agent
bind '127.0.0.1'
port #{@port}
port 24200
Copy link
Member

Choose a reason for hiding this comment

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

Why use fixed port number?
@port = unused_port is better?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just smallest diff to reduce warnings.
But you're right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated with unused_port.

@tagomoris tagomoris force-pushed the fix-tests-and-reduce-warnings branch from 2d94bdd to 578f002 Compare August 31, 2016 03:40
@tagomoris
Copy link
Member Author

I rebased on current master branch, and add a commit to edit upper limits of dependencies.

@repeatedly
Copy link
Member

LGTM

@tagomoris tagomoris force-pushed the fix-tests-and-reduce-warnings branch from a2d032e to 39541aa Compare August 31, 2016 10:12
@tagomoris tagomoris merged commit ec9960d into master Aug 31, 2016
@tagomoris tagomoris deleted the fix-tests-and-reduce-warnings branch August 31, 2016 10:49
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