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.

Sign up for GitHub

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

Jump to bottom

Address some Ruby warnings #1995

Merged
merged 5 commits into from
Feb 25, 2020
Merged

Address some Ruby warnings #1995

merged 5 commits into from
Feb 25, 2020

Conversation

nbeyer
Copy link
Contributor

@nbeyer nbeyer commented Feb 19, 2020

  • Add warnings enablement to spec_helper.rb to see future warnings
  • Use instance_variable_defined? to clean up not initialized warnings
  • Add 'inherited' to NON_OVERRIDABLE list
  • Add multi_xml to test dependencies in Gemfile to address spec failure

@nbeyer nbeyer mentioned this pull request Feb 19, 2020
Closed
dblock
dblock approved these changes Feb 21, 2020
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This looks good, needs a rebase please.

CHANGELOG.md Outdated Show resolved Hide resolved
@nbeyer
Address some Ruby warnings
11cfdd7 
* Add warnings enablement to spec_helper.rb to see future warnings
* Use instance_variable_defined? to clean up not initialized warnings
* Add 'inherited' to NON_OVERRIDABLE list
* Add multi_xml to test dependencies in Gemfile to address spec failure
@nbeyer
Add changelog entry
c32227a
@nbeyer
expand changelog entry
e34d082
@nbeyer
Copy link
Contributor Author

nbeyer commented Feb 24, 2020

@dblock This PR has been rebased.

dblock
dblock requested changes Feb 24, 2020
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

The multi_xml thing looks suspicious. Otherwise good to go.

Gemfile Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@dblock dblock merged commit 53d826f into ruby-grape:master Feb 25, 2020
@dblock
Copy link
Member

dblock commented Feb 25, 2020

Merged, thank you.

@dblock
Copy link
Member

dblock commented Feb 25, 2020

Now that we run with --warnings, how far are we from making tests fail on unexpected warnings?

@nbeyer nbeyer deleted the undefined_warnings branch February 25, 2020 15:56
@nbeyer
Copy link
Contributor Author

nbeyer commented Feb 25, 2020

I've not found anything great for failing tests based on unexpected warnings. It's possible to plug into Ruby's Warning module, but it would require some framework building, as it seems likely that warnings from dependencies are going to pop-up, which would be out of the control of this project.

basjanssen pushed a commit to basjanssen/grape that referenced this pull request Feb 28, 2020
@nbeyer @basjanssen
Address some Ruby warnings (ruby-grape#1995)
ae5be28 
* Address some Ruby warnings

* Add warnings enablement to spec_helper.rb to see future warnings
* Use instance_variable_defined? to clean up not initialized warnings
* Add 'inherited' to NON_OVERRIDABLE list
* Add multi_xml to test dependencies in Gemfile to address spec failure

* Add changelog entry

* expand changelog entry

* adjust changelog

* remove unnecessary multi_xml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nbeyer @dblock