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 spec warnings #1583

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

axfcampos
Copy link

Hi all,

When running specs with warnings on, grape outputs a lot of warnings.

This PR attempts to fix all of them.

With regards to the warnings: warning: instance variable @disable_warnings not initialized, by the hashie gem, I have opened a PR there to fix those: hashie/hashie#416

@dblock
Copy link
Member

dblock commented Feb 27, 2017

Interesting. This feels like a workaround rather than a fix. Why wouldn't something like @status for example not be defined?

@axfcampos
Copy link
Author

Hi @dblock the warnings occur because @status is in fact not defined when it is accessed. This may only happen in the specs...

I did not look into implementation details, my objective was to clear three warnings that were showing in my projects specs, but since I'm here might as well fix them all 😄

Having warnings not suppressed has saved me from some bugs in the past, but it comes with the caveat of having to fix them if we want a sane output when running tests 😛

Would you like me to change something on the PR?

@dblock
Copy link
Member

dblock commented Feb 27, 2017

Yes, lets have this discussion in hashie/hashie#416 first? Same problem but fewer changes.

@namusyaka
Copy link
Contributor

Thanks for your contribution.
I'd like to postpone this until after 1.0.0 release is made.

@dblock
Copy link
Member

dblock commented Aug 12, 2017

I want to revive this. I think a fix that says if defined(:@foobar) is not what we want. Lets initialize variables where needed instead? What do you think @axfcampos?

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.

3 participants