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

Fix uninitialized instance variable / overridden function warnings. #350

Merged
merged 2 commits into from
May 25, 2016
Merged

Fix uninitialized instance variable / overridden function warnings. #350

merged 2 commits into from
May 25, 2016

Conversation

xavierholt
Copy link
Contributor

Found myself cleaning up some Ruby warnings while testing a branch earlier today, so I thought I'd finish the job and help out with #344

Couple notes:

  • Most changes are simply setting things to nil in initialize methods.
  • Identical unindent functionality is provided by the unindent gem, so the monkeypatch version isn't needed any more.
  • Part of my fix makes group_size and wait_interval publicly readable on the runners. It seemed odd to have them writeable but not readable, so I felt okay making the visibility change. If that's not okay, let me know and I'll revert that part.

@leehambley
Copy link
Member

LGTM :) thanks for the contribution - Should I merge right now, or would you like to credit yourself in the changelog ?

@robd
Copy link
Contributor

robd commented May 21, 2016

Hey - apologies for chiming in out of nowhere!

One thing I noticed here (unless I missed something) is that the unindent method is only called in one place (test/functional/backends/test_netssh.rb). I wondered if it would be better to remove the SSHKit dependency on the unindent Gem, and move the unindent method from test/unit/core_ext/test_string.rb to test_netssh.rb?

I guess the only other thing to check is whether the build is passing - see discussion here: #351

@xavierholt
Copy link
Contributor Author

xavierholt commented May 21, 2016

@robd: Updated with an even further simplification - scrap the unindent method entirely!

@leehambley: No need - I got credit for my actual bugfix, and this is just trivial cleanup. I'm okay to merge whenever. Not sure if #351 is relevant. It'll pass CI; the issue only shows up when running on a host with a non-empty known_hosts file (I think). Just let me know if you'd like me to rebase after the fix is in.

@robd
Copy link
Contributor

robd commented May 21, 2016

scrap the unindent method entirely!

👍 Even better!

@mattbrictson
Copy link
Member

#351 is now fixed in master. Can you rebase, please?

@xavierholt
Copy link
Contributor Author

@mattbrictson: Rebased. Cheers!

@mattbrictson
Copy link
Member

Acceptance tests pass. ✅

@leehambley I'll leave the merge to you.

@leehambley
Copy link
Member

Thanks @xavierholt and for the sanity checking @mattbrictson

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