-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add grape controller module with test application #16
Add grape controller module with test application #16
Conversation
|
||
def endpoint_for(name, config_source: self.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for removing this config_source
arg? 🌷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you override the entire method instead, for grape.rb
and change the defaulting there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, do you mean override endpoint_for
itself ?
We actually need config_source
in other methods too (API.endpoint
and compile_options_for_controller
).
lib/trailblazer/endpoint.rb
Outdated
@@ -145,4 +145,5 @@ def self.advance_from_controller(endpoint, success_block:, failure_block:, proto | |||
require "trailblazer/endpoint/adapter" | |||
require "trailblazer/endpoint/dsl" | |||
require "trailblazer/endpoint/controller" | |||
require "trailblazer/endpoint/grape/controller" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we require that for grape environments, only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I've removed it.
82052a7
to
77b1fab
Compare
77b1fab
to
23f6e9f
Compare
23f6e9f
to
1631d40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be an option to use the Rails app/concepts
in the Grape tests? I am thinking of the far future, where we have lots of code to test in both frameworks.
Good point, I'll look into it. |
Trailblazer::Endpoint::Grape::Controller
to make.endpoint
,.directive
&#endpoint
available in Grape classesGrape::API
classes work. They're not copying@normalizers
settings.