Skip to content

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 thogg4 force-pushed the silence-warnings branch from dbe8be1 to 14baca7 Compare May 12, 2017 15:42
@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
@thogg4 thogg4 force-pushed the silence-warnings branch from 14baca7 to 8bc30ad Compare May 12, 2017 17:10
@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