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

Allows to set a global ParamBuilder #1833

Merged
merged 15 commits into from
Dec 10, 2018
Merged

Allows to set a global ParamBuilder #1833

merged 15 commits into from
Dec 10, 2018

Conversation

myxoh
Copy link
Member

@myxoh myxoh commented Dec 5, 2018

This is related to the issue: #1775

Creates a new module for global configuration (I checked the documentation and couldn't find one available), and added the setting for a global param builder

darren987469
darren987469 reviewed Dec 6, 2018
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@dblock
Copy link
Member

dblock commented Dec 6, 2018

If we want to add global configuration to Grape lets adopt the Ruby-esque standard like all the other gems. I should be able to do

Grape.configure do |config|
   config.x = 
end

Take a look at https://github.com/dblock/strava-ruby-client/blob/master/lib/strava/api/config.rb for an example.

@myxoh
Copy link
Member Author

myxoh commented Dec 6, 2018

If we want to add global configuration to Grape lets adopt the Ruby-esque standard like all the other gems. I should be able to do

Grape.configure do |config|
   config.x = 
end

Take a look at https://github.com/dblock/strava-ruby-client/blob/master/lib/strava/api/config.rb for an example.

Not particularly a fan of it (which is why it wasn't my first choice), but should be fairly easy to implement, and I see your point 👍
I'll do so between later today, and early tomorrow.
Other than that, any point you want to add?

@dblock
Copy link
Member

dblock commented Dec 6, 2018

Ruby is lovely because it's often convention over configuration, and predictable imperfect way of doing things consistently. Long live Ruby :)

Code looks good otherwise so far. I'll re-review thoroughly when you have the config stuff sorted out.

Thanks!

@myxoh myxoh mentioned this pull request Dec 7, 2018
Closed
@myxoh
Copy link
Member Author

myxoh commented Dec 7, 2018

@dblock I've made those changes

@myxoh myxoh mentioned this pull request Dec 7, 2018
Closed
dblock
dblock requested changes Dec 7, 2018
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.

See notes.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
dblock
dblock reviewed Dec 7, 2018
View reviewed changes
spec/grape/config_spec.rb Outdated Show resolved Hide resolved
dblock
dblock approved these changes Dec 10, 2018
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.

Looks good. I made some minor suggestions for language in README. Feel free to make/not make them and merge (I would squash since the change ended up being quite minimal).

README.md Outdated
@@ -541,6 +542,29 @@ end

[grape-swagger]: https://github.com/ruby-grape/grape-swagger

## Configuration

Grape counts with a module `Grape.configure` for some basic configuration at load time.
Copy link
Member

Choose a reason for hiding this comment

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

Counts is probably the wrong word. I tend to keep this as short as possible. For example:

Use Grape.configure to setup global settings at load time.

README.md Outdated
@@ -618,6 +642,8 @@ params do
end
```

Or globally with the [Configuration](#configuration) `Grape.configure.param_builder`
Copy link
Member

Choose a reason for hiding this comment

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

Missing a period.

README.md Outdated
Grape counts with a module `Grape.configure` for some basic configuration at load time.
Currently the configurable settings are:

* `param_builder`: Sets the default [Parameter Builder](#parameters), by default: `Grape::Extensions::ActiveSupport::HashWithIndifferentAccess::ParamBuilder`
Copy link
Member

Choose a reason for hiding this comment

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

Sets the Parameter Builder, defaults to Grape::Extensions::ActiveSupport::HashWithIndifferentAccess::ParamBuilder

@dblock
Copy link
Member

dblock commented Dec 10, 2018

We do need a green CI. Didn't look at failures.

@myxoh
Copy link
Member Author

myxoh commented Dec 10, 2018

There seems to be an independent failure on the Gem dependencies. I'll try to address it on a separate PR

@myxoh
Copy link
Member Author

myxoh commented Dec 10, 2018

Opened issue for this: #1839 It seems to be related with coveralls which is no longer maintained

@dblock dblock merged commit 766cdb5 into master Dec 10, 2018
@dblock
Copy link
Member

dblock commented Dec 10, 2018

👍

basjanssen pushed a commit to basjanssen/grape that referenced this pull request Feb 28, 2020
@myxoh @basjanssen
Allows to set a global ParamBuilder (ruby-grape#1833)
7cdfc31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darren987469 darren987469 darren987469 left review comments

@dblock dblock dblock approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@myxoh @dblock @darren987469