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
[Controller] Adds Route Helper Methods to Controller #362
Conversation
> As a developer I want to have access to the current matched route information such as the Action, Controller, Resource and Scope accessed. This adds a Route helper file - Includes the following methods route_action, route_controller, route_resource, route_scope in the controller ### Alternate Design I thought of using a different name such as action_name, controller_name but it seemed more explicit to have `route_` prefix to hint the user where is this information is coming from.
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.
Overall, looks good.
The regex should be changed, but all other comments are optional.
end | ||
|
||
def controller_name | ||
self.class.name.gsub(/Controller/i, "") |
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.
For this regex, I would anchor to the end. And does the i
make it case-insensitive, is that necessary?
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 people want the controller name downcased?
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.
Yeah, I think we should use /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.
You guys are correct I will also fix the regex. I looked at the rails implementation
https://apidock.com/rails/ActionController/Metal/controller_name/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.
Use sub instead of gsub since we only want to replace once.
spec/amber/controller/base_spec.cr
Outdated
controller.responds_to?(:route_action).should eq true | ||
controller.responds_to?(:route_resource).should eq true | ||
controller.responds_to?(:route_scope).should eq true | ||
controller.responds_to?(:route_controller).should eq true |
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.
Do all of these need to be prefixed with route_
?
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.
I prefixed to avoid name collisions with methods define by the user. I am open to suggestions
* Adds Route Helper Methods to Controller > As a developer I want to have access to the current matched route information such as the Action, Controller, Resource and Scope accessed. This adds a Route helper file - Includes the following methods route_action, route_controller, route_resource, route_scope in the controller ### Alternate Design I thought of using a different name such as action_name, controller_name but it seemed more explicit to have `route_` prefix to hint the user where is this information is coming from. * fixup! Adds Route Helper Methods to Controller
* Adds Route Helper Methods to Controller > As a developer I want to have access to the current matched route information such as the Action, Controller, Resource and Scope accessed. This adds a Route helper file - Includes the following methods route_action, route_controller, route_resource, route_scope in the controller ### Alternate Design I thought of using a different name such as action_name, controller_name but it seemed more explicit to have `route_` prefix to hint the user where is this information is coming from. * fixup! Adds Route Helper Methods to Controller Former-commit-id: 3df8c1e
This adds a Route helper file
route_resource, route_scope in the controller
Alternate Design
I thought of using a different name such as action_name, controller_name
but it seemed more explicit to have
route_
prefix to hint the user where isthis information is coming from.