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 warnings #254

Closed
wants to merge 6 commits into from
Closed

Fix warnings #254

wants to merge 6 commits into from

Conversation

tfausak
Copy link
Contributor

@tfausak tfausak commented Sep 10, 2015

I encountered some warnings in this gem while reviewing bolshakov/stoplight#78. This pull request fixes those warnings. I chose the path of least resistance for most of these fixes. Since I don't know this library, there may be better ways to fix them.

Ultimately I think the only commit I really care about is 5647d23. I think it's causing my JRuby build to fail.

@kattrali
Copy link
Contributor

Hi @tfausak! Thank you for the contribution. I took a look at the crash in JRuby and can confirm the crash and the fix. In addition, shadowing the config variable is undesirable and might lead to weird bugs later.

Regarding the use of uninitialized instance variables, this seems to be a fairly standard pattern in Ruby. Have you seen anything in particular which is broken by this syntax? In the meantime, I'm merging this pull request with the caveat of removing 022c3e1 (uninitialized instance variables) and e7aec19 (enable warnings by default). Enabling all warnings turns on a fair amount of chatter from dependencies.

Merged manually in a041d1f.

@tfausak
Copy link
Contributor Author

tfausak commented Nov 16, 2015

Using uninitialized instance variables is common in Ruby. I think that is unfortunate, however I don't think it breaks anything. My main motivation for initializing instance variables in this gem was to avoid spurious warning when running the tests in Stoplight. The "chatter" you see when enabling warnings is the same chatter that I see in my gem — and I'm trying to reduce it.

@kattrali
Copy link
Contributor

Using uninitialized instance variables is common in Ruby. I think that is unfortunate, however I don't think it breaks anything.

Ah, thank you for the confirmation.

My main motivation for initializing instance variables in this gem was to avoid spurious warning when running the tests in Stoplight. The "chatter" you see when enabling warnings is the same chatter that I see in my gem — and I'm trying to reduce it.

Gotcha. I understand not wanting it in your output and share the sentiment. From the bugsnag gem side, enabling warnings picks up a few things in webmock and httpclient which are not ideal but did not seem harmful. I split the dependency warnings into a separate issue to address later (#257). Thank you for following up.

@tfausak
Copy link
Contributor Author

tfausak commented Nov 16, 2015

Thanks!

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