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

Avoid noise when code runs with Ruby warnings #251

Merged
merged 1 commit into from
Nov 28, 2016

Conversation

cpetschnig
Copy link
Contributor

New versions of RSpec have warnings turned on by default:

# This setting enables warnings. It's recommended, but in some cases may
# be too noisy due to issues in dependencies.
config.warnings = true

I believe these warnings are helpful in general to write better code. However, Grape Entity is very noisy. I get more than 10 lines of warnings for one single test where Grape Entity is involved.

I hope you agree. Some of my changes could also be solved differently for sure. I am happy to discuss and change those cases. I also turned warnings on in the test suite.

@@ -2,11 +2,11 @@

#### Features

* Your contrbution here.
* Your contribution here.
Copy link
Member

Choose a reason for hiding this comment

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

thanks


#### Fixes

* Your contrbution here.
Copy link
Member

Choose a reason for hiding this comment

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

please re-add it below your contribution

@LeFnord
Copy link
Member

LeFnord commented Nov 25, 2016

thanks @cpetschnig for contributing, please see my comment above

config.raise_errors_for_deprecations!

config.warnings = true
end
Copy link
Member

Choose a reason for hiding this comment

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

please revert this change

Copy link
Member

Choose a reason for hiding this comment

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

@LeFnord maybe this should be enabled?
@ cpetchnig do we get a pass with all the changes you made and another setting to fail the build if warnings? if so I recommend doing that

Copy link
Member

Choose a reason for hiding this comment

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

@dblock this would enable the warnings and makes also the specs noisy, maybe they could be used for writing better code, but think for that we have rubocop, so by enabling it, we have two of such tools … mmh

if they both give the same or similar warnings, then one is enough and the rubocop messages much more helpful, IMHO the rspec warnings are higher level and helpful for experienced devs, but they also raises the barrier for contributions

if not then it should be disabled

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @LeFnord.

New versions of RSpec have warnings turned on by default:

    # This setting enables warnings. It's recommended, but in some cases may
    # be too noisy due to issues in dependencies.
    config.warnings = true

I believe these warnings are helpful in general to write better code. However, Grape Entity is very noisy. I get more than 10 lines of warnings for one single test where Grape Entity is involved.

I hope you agree. Some of my changes could also be solved differently for sure. I am happy to discuss and change those cases. I also turned warnings on in the test suite.
@grape-bot
Copy link

<tr>
  <td>:warning:</td>
  <td data-sticky="false">There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.</td>
</tr>
1 Warning

Generated by 🚫 danger

@cpetschnig
Copy link
Contributor Author

Thank you for your feedback. I have updated the PR.

@LeFnord
Copy link
Member

LeFnord commented Nov 28, 2016

👍

@LeFnord LeFnord merged commit 447cc03 into ruby-grape:master Nov 28, 2016
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