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

Use defined? to correct not initialized warnings #1909

Closed
wants to merge 3 commits into from
Closed

Use defined? to correct not initialized warnings #1909

wants to merge 3 commits into from

Conversation

nbeyer
Copy link
Contributor

@nbeyer nbeyer commented Sep 27, 2019

The class instances variables @versions and @namespace_description may not be initialized when used and Ruby will report their access as a warning. Using the defined? keyword will correct that. There are plenty of alternative changes could be made to avoid this, but I just picked the most minimal change.

The class instances variables `@versions` and `@namespace_description` may not be initialized when used and Ruby will report their access as a warning. Using the defined? keyword will correct that. There are plenty of alternative changes could be made to avoid this, but I just picked the most minimal change.
@dblock
Copy link
Member

dblock commented Sep 30, 2019

I was never able to see these. How do you reproduce the warnings?

Needs a CHANGELOG entry. There's also something about the octokit plugin & danger going south, can you please take a look and make sure the build is green? hashie/hashie#486 was having a similar problem.

@nbeyer
Copy link
Contributor Author

nbeyer commented Oct 1, 2019

From the command-line, warnings can be turned on by adding the '-w' switch.

From RSpec, warnings can be turned on in the config (i.e. spec_helper.rb), like this:

RSpec.configure do |config|
  # ...
  config.warnings = true
  # ...
end

@grape-bot
Copy link

grape-bot commented Oct 1, 2019

1 Warning
⚠️ There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by 🚫 danger

@nbeyer
Copy link
Contributor Author

nbeyer commented Oct 1, 2019

@dblock I have the builds running cleanly now. Let me know if you'd like me to add the warnings enablement to the spec_helper.rb.

Note - this method redefinition causes a lot of warnings - https://github.com/ruby-grape/grape/blob/master/lib/grape/api.rb#L57

@dblock
Copy link
Member

dblock commented Oct 2, 2019

@nbeyer Are you saying we can have a clean test run without warnings? If so we should turn on warnings as errors (the answer to your question is "yes, please").

@dblock
Copy link
Member

dblock commented Dec 2, 2019

@nbeyer want to finish this?

@nbeyer
Copy link
Contributor Author

nbeyer commented Feb 19, 2020

Abandoning this for #1995

@nbeyer nbeyer closed this Feb 19, 2020
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.

None yet

3 participants