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

Add Rails/SpecificActionNames cop #827

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

r7kamura
Copy link
Contributor

DHH says that only default CRUD action names should be used and any other action would lead to the creation of a dedicated controller (which itself only has default CRUD actions):

And in fact, I think rubocop-rails somewhat expects these default action names to be used in its Rails/ActionOrder cop.

ExpectedOrder:
- index
- show
- new
- edit
- create
- update
- destroy

With this background in mind,, I think it would be nice to have such a cop available.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

config/default.yml Outdated Show resolved Hide resolved
module RuboCop
module Cop
module Rails
# Use only specific action names.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the background of this PR description is written, the intention will be conveyed to users.

# def index
# end
# end
class ActionName < Base
Copy link
Member

@koic koic Oct 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RestfulActionName , or something like that might be more expressive.

Copy link
Contributor Author

@r7kamura r7kamura Oct 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I'd like to discuss the name some more. This is a topic that will be relevant to the detailed description I will write in the Cop comments.

I am considering the following three or more candidates for the name of this Cop:

  1. RestfulActionName
  2. SpecificActionName
  3. ActionName

The original background and motivation is that I want to use only the default action names for CRUD, but what this Cop is actually doing is just limiting the action names to specific predefined name set.

For example, if a convention emerged that there should be one controller class per endpoint, we could do that by limiting all action names to #call or something. Like this, the usage of this Cop is not necessarily limited to RESTful action names. This is why I gave it the rather generic name "ActionName."

If we believe that Cop name should be in keeping with the original good practice or idea, then "Restful" (or "Default" ?) is a good name to use. Otherwise, if we are naming Cop with a focus on what it actually does, we would avoid "Restful" in this case.

If "ActionName" seems too generic, then I would suggest changing the name to something like "SpecificActionName" or "PredefinedActionName".

Copy link
Contributor

@andyw8 andyw8 Oct 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the concept of this, but it's a tricky to decide what to call it.

I think we should avoid having Name as part of the cop name. This isn't really about how you name the methods, it's about how you structure the app.

My suggestion is Rails/ConstrictedActions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next release is a bugfix anyway. Let's take a moment to think of a more better name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comments about the cop naming.

I think Rails/ActionName is a bit too abstract and would be better to be at least more concrete. In that sense, I agree with the direction of Rails/ConstrictedActions. For now, I have chosen the Cop name Rails/SpecificActionNames, after the vocabulary of the offense message.

I don't think we should not force the RESTful concept too strongly on users, considering the users who use this simply to reduce the variety of method names. It is difficult to nuance that, but it would be nice for the Cop to say something like "The default is RESTful oriented and we think it should be so if possible, but it is not mandatory".

@@ -98,6 +98,21 @@ Rails/ActionFilter:
- app/controllers/**/*.rb
- app/mailers/**/*.rb

Rails/ActionName:
Description: Use only specific action names.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to be a little more specific about "specific".

expect_offense(<<~RUBY)
class UsersController < ApplicationController
def articles
^^^^^^^^ Use only specific action names.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the message should include the list of configured actions, e.g.:

Use only specific action names: index, create, show...

@r7kamura r7kamura changed the title Add Rails/ActionName cop Add Rails/SpecificActionNames cop Nov 2, 2022
@koic
Copy link
Member

koic commented Apr 4, 2023

Note, It may be first how the discussion of rubocop/rails-style-guide#290 will be reflected in the style guide.

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

Successfully merging this pull request may close these issues.

None yet

3 participants