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

Silence warnings #1632

Merged
merged 5 commits into from
Jun 12, 2017
Merged

Silence warnings #1632

merged 5 commits into from
Jun 12, 2017

Conversation

thogg4
Copy link
Contributor

@thogg4 thogg4 commented May 10, 2017

@dblock maybe something like this?

fixes #1582

@dblock
Copy link
Member

dblock commented May 12, 2017

Better! Lets finish this.

If this lets us turn on warnings as errors in the build that should be part of this.

If it fully resolves #1582, it needs a CHANGELOG entry.

Fix the build, needs rubocop -a and rubocop --auto-gen-config.

Remove things that are unrelated to this PR.

Squash.

@thogg4
Copy link
Contributor Author

thogg4 commented May 12, 2017

@dblock how is that?

initialize vars in initializers

subclass hashie mash to silence warnings

rubocop fixes

add changelog entry

Revert "use correct params class in declared"

This reverts commit 61f0c8e.

fix tests
@dblock
Copy link
Member

dblock commented May 12, 2017

This looks great. Before I merge this, what is the effect of disable_warnings when using Mash? This is different from uninitialized variables warnings, I am not sure we want to lump this together.

@thogg4
Copy link
Contributor Author

thogg4 commented May 18, 2017

@dblock that is in conjunction with this pull request in hashie: hashie/hashie#395

They have you subclass Mash so you can disable warnings that you cause when you use it.

@dblock
Copy link
Member

dblock commented May 18, 2017

I understand but why do we ever want this? Don't you want to know when you're assigning a parameter that overrides something built in?

@thogg4
Copy link
Contributor Author

thogg4 commented Jun 12, 2017

@dblock you're right. i have removed that.

@dblock
Copy link
Member

dblock commented Jun 12, 2017

I'll wait on 🍏 to re-review.

@grape-bot
Copy link

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

Generated by 🚫 danger

@dblock dblock merged commit 5ed770f into ruby-grape:master Jun 12, 2017
@dblock
Copy link
Member

dblock commented Jun 12, 2017

Merged, thanks.

thogg4 added a commit to thogg4/grape that referenced this pull request Jun 12, 2017
use correct params class in declared

add changelog entry

add tests for declared class

fix rubocop

make changelog entry better

remove puts in tests

one more puts

change test names

conform changelog entry

return dynamic class name

parse response instead

remove params

Instrument validators with ActiveSupport::Notifications

Suppress `warning: method redefined`

Bugfix: Correctly handle `given` in Array params (ruby-grape#1625)

* Bugfix: Correctly handle `given` in Array params

Array parameters are handled as a parameter that opens a
scope with `type: Array`; `given` opens up a new scope, but
was always setting the type to Hash. This patch fixes that,
as well as correctly labeling the error messages associated
with array fields.

* Code review fix

* Add CHANGELOG entry

Add ability to include an array of modules as helpers

Changing helpers DSL to allow the inclusion of many modules.
This attemps to bring a better readability, since it seems to be
more intuitive to send a list of modules when the message in
question is called helpers.

ensure we return a string from test

return json in tests

Update README according to new `helpers` macro interface

Silence warnings (ruby-grape#1632)

* silence warnings

initialize vars in initializers

subclass hashie mash to silence warnings

rubocop fixes

add changelog entry

Revert "use correct params class in declared"

This reverts commit 61f0c8e.

fix tests

* remove disable_warnings in hashie mash

* make rubocop happy

* fix hashie tests
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.

warnings when running tests
3 participants