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

accepting proc for options #441

Merged
merged 1 commit into from
May 28, 2016
Merged

accepting proc for options #441

merged 1 commit into from
May 28, 2016

Conversation

LeFnord
Copy link
Member

@LeFnord LeFnord commented May 28, 2016

this solves #429

host and base_path accepts String, proc or lambda

updates readme and changelog
@@ -216,6 +220,8 @@ add_swagger_documentation \
base_path: '/super/api'
```

`host` and `base_path` are also accepting a `proc` or `lambda`
Copy link
Member

Choose a reason for hiding this comment

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

Make it into a full sentence, like "The host and base_path options also accept ..".

Copy link
Contributor

@Lordnibbler Lordnibbler May 29, 2016

Choose a reason for hiding this comment

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

In addition to that, I think providing some trivial usage example would be super helpful in the docs here:

add_swagger_documentation(
  base_path: proc { |foo| foo.host =~ /^example/ ? '/api-example' : '/api' }
)

@dblock
Copy link
Member

dblock commented May 28, 2016

I am going to merge this, my comments are more like next steps. We can still change things wrt my arity comment, but I'd be ok with shipping it as is too.

@dblock dblock merged commit b38f2be into ruby-grape:master May 28, 2016
@LeFnord
Copy link
Member Author

LeFnord commented May 28, 2016

yeap … good point
to your last comment, also yeap, agreeing, but also thinking issues are not enough for that,
better something like tickets, card, whatever, what is better suitable for developing ideas

@Lordnibbler
Copy link
Contributor

Sure, thanks for addressing this; I left a small comment suggesting we have a usage example for the base_path option using a proc in the docs so folks dont necessarily have to dig in the tests/source

@dblock
Copy link
Member

dblock commented May 29, 2016

@Lordnibbler do PR that!

@Lordnibbler
Copy link
Contributor

Sure

@Lordnibbler
Copy link
Contributor

#442

@Lordnibbler Lordnibbler mentioned this pull request May 31, 2016
LeFnord pushed a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants