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

Disable Style/MethodCallWithArgsParentheses cop for migrations #674

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

obie
Copy link

@obie obie commented Jan 22, 2025

Same rationale as disabling this cop for Gemfile (the source is a DSL and reads worse with parentheses)

obie and others added 2 commits January 22, 2025 12:10
This automated commit dumps the contents of the full RuboCop config.
[dependabot skip]
@obie obie requested a review from a team as a code owner January 22, 2025 18:13
@github-actions github-actions bot added the config change Changes the Rubocop config by enabling, disabling, or reconfiguring one or many cops label Jan 22, 2025
@obie obie requested a review from rafaelfranca January 22, 2025 18:14
@@ -635,6 +635,7 @@ Style/MethodCallWithArgsParentheses:
- puts
Exclude:
- "/**/Gemfile"
- "/**/db/migrate/*"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I am not sure if a blanket exclusion for migration files is the best thing to do. We want to exclude only because things like create_table, etc read better as DSLs, but people are not limited to writing those in migration files, and we probably do want to lint all other method calls.

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 22, 2025

We decided to not make distinctions with DSLs. Everything should have parentheses. #100 (comment)

#106 (comment)

@rafaelfranca
Copy link
Member

I wonder if we can see why migrations aren't considered macros that are allowed. I'm ok with this exception, but I'd like to maybe not add exceptions and maybe teach rubocop about those kind of macros.

@sambostock
Copy link
Contributor

Using an example migration from the Rails docs:

# db/migrate/20240502100843_create_products.rb
class CreateProducts < ActiveRecord::Migration[8.0]
  def change
    create_table :products do |t|
      t.string :name
      t.text :description

      t.timestamps
    end
  end
end

I believe "macros" are loosely defined as methods called within a module/class body:

class Post < ApplicationRecord
  belongs_to :user # macro
end

User = Class.new(ApplicationRecord)
User.has_many(:posts) # not macro

By that definition, nothing in that migration is a "macro".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config change Changes the Rubocop config by enabling, disabling, or reconfiguring one or many cops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants