-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
7 Standard Controller Actions #290
Comments
Another useful education link: http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/ |
And how is it going so far, what challenges you on this way?
Not to mention the non-resourceful routes:
Is there evidence that among real-world Rails apps there's a tendency to stick to those seven default methods? |
About non-resourceful routes: |
For the reference, ROCA-style. |
I can't seem to find the paragraphs relevant to this discussion in the link, can you help me out? |
I believe you mistakenly posted the same link (posted above) twice.
Indeed, justifying a significant pivot can be hard, and it's hard to do in isolation. Still, if the guide recommends something, the "those two guys think it's the right approach" might not be sufficient. Personally, I think there's a threshold when it makes sense to extract an action from the otherwise resourceful controller. E.g. However, for e.g.
This is reasonable in most cases. However, what if it's a controller with no base resourceful actions at all, and 3 custom actions? What I'm trying to say is that applications differ a lot, and there are different programming styles and architecture patterns.
We are not discussing the cop here. We're deciding if a certain practice is widespread enough to be added as a guideline.
|
You're right, I have fixed the link.
I agree, there's a threshold.
I would always prefer either:
I have never seen a case where I would prefer 3 custom actions to splitting into more controllers with standard actions. If such a case exists that would make a very convincing case for not doing the cop. That's why I think this is a good idea for a guide - It's not obvious before you do it, but always it turns out to be the right thing to split up into more controllers and use standard actions.
Like a true believe, I would preach that not following ones natural urges, and restricting oneself to the 7 resourceful actions would lead to better, more idiomatic code.
There's a bit of a Chicken and Egg problem here though - Having the cop would probably lead to more adoption of the pattern. I've found using the standard actions to lead to better code. I don't have any evidence. I suggest we leave this thread here for a while and see if it gets attention from people with the same conviction as me. Otherwise, I guess it's too much of a fringe opinion to be included in the style guide.
Very interesting, I'll play around with this. |
@mollerhoj Have you had a chance to digest real-world-rails? I've recently found it comes with a tool, |
In curious search of that kind of cop I found this thread. I don't feel that I can fully involve in the discussion now. So I say only some points why we use only standard actions:
|
I agree with @mollerhoj that we should make this the recommended approach. My reasoning is a little bit different, though. We've been able to greatly simplify a production Rails app just by separating stuff out into smaller, more focused controllers. This is essentially the idea behind limiting the number of actions to just the most basic CRUD ones. If the number of actions is limited, you're forced to think about resource operations in a way that requires separating them into sub-resources. The last sentence might be hard to grok without a concrete example: Here's an example where custom actions are used: class RecordsController
def index
end
def update_positions
end
def bulk_destroy
end
end The problem with this is not the actions themselves, it's the instantiation and all auxiliary code required to make those actions work. They most likely forces a violation of https://rails.rubystyle.guide/#one-method. Here's a couple of reasons why I think this is problematic:
module Records
class PositionsController
def update
end
end
end
module Records
class BulkController
def destroy
end
end
end Spacing things out solves all four problems I outlined above. I also toyed with the idea of being lenient on whether or not we should fundamentalistically (yes that's a word, I checked) adhere to this rule. My knee jerk reaction based on common sense was to be lenient, but that only lead to more confusion than the other option, so we adopted the fundamentalistic approach here. This is a whole other are of discussion, but I'd like to briefly draw a parallel between controllers and models. It's idiomatic rails to separate code for one piece of functionality into a concern and include it in the main model. This is exactly the same as separating parts of the controller, specific to a subset of functionality into a separate controller. Finally it's also related to the Rails doctrine. Even though both "Convention over Configuration" and "Push up a big tent" are part of it. "Convention over Configuration" is at second place and "Push up a big tent" is last. This is related to the discussion here, because we're discussing adopting a convention that has objective benefits, versus being lenient on it to not discourage other ways to approach the problem. I'm strongly opinionated that we should implement this cop. I'm not excluding the possibility that there's certain cases where this might not be practical, but that's exactly why we have the ability to disable a cop. |
@koic Are we looking to reach a consensus here in order to be able to move forward with the associated cop (rubocop/rubocop-rails#827)? |
Limiting public controller actions to the 7 standard actions prevents fat controllers and generally enforces better domain modeling, as demonstrated in this talk: https://www.youtube.com/watch?v=HctYHe-YjnE
I would like to see a rubocop that enforces that the 7 standard actions are only public methods in controllers.
This will work nicely with #289
The text was updated successfully, but these errors were encountered: