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

Reset endpoint options between calls #918

Closed
wants to merge 1 commit into from

Conversation

yesmeck
Copy link
Contributor

@yesmeck yesmeck commented Feb 10, 2015

I'm using Grape and Rable with grape-rabl gem, I have following code:

class Comments < Grape::API
  resources :comments do
    post do
      comment = Comment.new(params)
      if comment.save
        render rabl: 'comments/show'
      else
        car.errors.to_hash
      end
    end
  end
end

When first request save the comment succesfully, whatever the nex request save the comment succesfully or not, the rabl template will alway be rendered. Because grape-rabl write the template info to the endpoint options (https://github.com/LTe/grape-rabl/blob/master/lib%2Fgrape-rabl%2Frender.rb#L4), so I thinks the endpoint options should be reset between calls.

@dblock
Copy link
Member

dblock commented Feb 10, 2015

I am 50/50 on this. Endpoint options could very well survive requests, since the endpoint isn't supposed to change. We could freeze it explicitly?

I think grape-rabl should be using ENV. Could you open an issue there, please?

Either way this needs a CHANGELOG and the rubocop build to be fixed, please.

@yesmeck
Copy link
Contributor Author

yesmeck commented Feb 10, 2015

If endpoint isn't supposed to change, I think we can just make the endpoint options read only.

I have open an issue on grape-rabl.

@yesmeck
Copy link
Contributor Author

yesmeck commented Feb 10, 2015

It seems grape-rabl is hard to store template info in env directly, because when we write code like get '/', rabl: 'index', the template info is have to be stored in route_options.

@dblock
Copy link
Member

dblock commented Feb 10, 2015

I think the declarative part should be stored on the endpoint, but the runtime part should not.

@yesmeck
Copy link
Contributor Author

yesmeck commented Feb 10, 2015

@dblock Yes, I figured it out, I'm composing a new PR to grape-rabl.

@yesmeck
Copy link
Contributor Author

yesmeck commented Feb 11, 2015

As ruby-grape/grape-rabl#36 merged, should we still need to freeze the @options or make it read only? If the answer is yes, grape will not compatible with old version grape-rabl before 0.3.1.

@dblock
Copy link
Member

dblock commented Feb 11, 2015

IMO, I think we should keep it as is.

@dblock
Copy link
Member

dblock commented Feb 11, 2015

Closing this, LMK if you think we should still do something here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants