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

DSL::Desc hash options are overriden by initializing them in block #706

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

Conversation

ysv
Copy link

@ysv ysv commented Sep 12, 2018

No description provided.

@coveralls
Copy link

coveralls commented Sep 12, 2018

Coverage Status

Coverage decreased (-0.1%) to 99.205% when pulling 97b3a0f on ysv:bugfix/issue-705 into d20b910 on ruby-grape:master.

def app
Class.new(Grape::API) do
desc 'Show this endpoint', nickname: 'simple'
desc 'Show this endpoint', nickname: 'simple' do
success Test
Copy link
Author

Choose a reason for hiding this comment

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

success overrides nickname so nickname is set to default value now

Copy link
Contributor

Choose a reason for hiding this comment

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

@ysv Hash options combined with block options is deprecated. We shouldn't do that. Instead, use

desc 'Show this endpoint', nickname: 'simple', success: Test

or

desc 'Show this endpoint' do
  nickname 'simple'
  success Test
end

The test will pass.

@ysv
Copy link
Author

ysv commented Sep 17, 2018

closed in ruby-grape/grape#1791

@dblock
Copy link
Member

dblock commented Oct 18, 2018

We should get this test merged after a passing build. Want to clean it up @ysv?

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.

5 participants