Fixed duplicating endpoints #988
Conversation
| .to change{ subject.endpoints.count }.from(0).to(1) | ||
| end | ||
|
|
||
| it 'uniq the duplicating endpoint' do |
There was a problem hiding this comment.
You don't need to put these two tests together, just add a new test at the same level that says 'does not duplicate identical endpoints'.
|
I understand the case of a reloader, but is this generally correct? What happens when you re-define an endpoint with all the same arguments, you probably would want the second one, not the first one, to be defined or to see a warning/error? Maybe a better approach could be to freeze endpoints or something like that? Not sure if it can be accomplished. Definitely needs a CHANGELOG, thanks. |
| @@ -120,7 +120,8 @@ def route(methods, paths = ['/'], route_options = {}, &block) | |||
| }).deep_merge(route_setting(:description) || {}).deep_merge(route_options || {}) | |||
| } | |||
|
|
|||
| endpoints << Grape::Endpoint.new(inheritable_setting, endpoint_options, &block) | |||
| new_endpoint = Grape::Endpoint.new(inheritable_setting, endpoint_options, &block) | |||
| endpoints << new_endpoint unless endpoints.any?{ |o| o.options == new_endpoint.options && o.inheritable_setting.to_hash == new_endpoint.inheritable_setting.to_hash } | |||
There was a problem hiding this comment.
I would want the comparison logic to live in Endpoint, so a comparison operator on Grape::Endpoint so we could maybe write endpoints.include?(...) or endpoints.any? { |e| endpoint.equals?(e) } # or ==.
There was a problem hiding this comment.
Yes, The the comparison of Endpoint is meaningful.
|
Yes.I can't agree more.Before, I did not think so much about that. |
|
Merging. |
#552