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 new Rails/ValidationsGrouping cop #679

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fatkodima
Copy link
Contributor

Closes #555.

Autocorrection is more involved, so I suggest to leave it as a future improvement.

Tested on gitlab's codebase: a bunch of offenses, no errors.

@fatkodima fatkodima force-pushed the validations-grouping-cop branch from b778056 to 723dc27 Compare March 22, 2022 12:02
@Bhacaz
Copy link
Contributor

Bhacaz commented May 17, 2022

@fatkodima
A suggestion, to simplify the autocorrection, maybe the config SingleAttributeValidations could be instead an other cop that can be disable if not wanted.

A cop with check and autocorrect

# bad
validates :email, format: { with: /@/ }
validates :email, presence: true
# good
validates :email, presence: true, format: { with: /@/ }

and the other

# bad
validates :name, :bio, presence: true
# good
validates :name, presence: true
validates :bio, presence: true

The combination of both cops will help to follow the style guide.

What do you think?

@fatkodima
Copy link
Contributor Author

@koic Can you review this?

@Earlopain
Copy link
Contributor

Can you show in tests how this handles things like this:

# class-level conditionals
if something?
  validates :email, presence: true
end

# or https://api.rubyonrails.org/classes/Object.html#method-i-with_options
with_options if: :something? do
  validates :email, presence: true
end

I don't believe there is code to handle that at the moment.

Also, can you add a test where validations have overlapping conditions:

validates :email, format: { with: /foo/ }
validates :email, format: { with: /bar/ }

@fatkodima fatkodima force-pushed the validations-grouping-cop branch from 723dc27 to 1b96385 Compare September 2, 2024 11:45
@fatkodima
Copy link
Contributor Author

Can you show in tests how this handles things like this

Made that validations inside each branch/etc are considered separately.

Also, can you add a test where validations have overlapping conditions

Added.

@fatkodima fatkodima force-pushed the validations-grouping-cop branch from 1b96385 to fb8c344 Compare September 2, 2024 11:48

it 'registers an offense when attribute validations have different validation options' do
expect_offense(<<~RUBY)
validates :email, format: { with: /foo/ }
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the expected correction for this case?

validates :bool, inclusion: { in: [false], if: :foo? }
validates :bool, inclusion: { in: [true], if: :bar? }

I can't come up with something that doesn't make this more complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

validates :bool, inclusion: { in: :some_meaningfull_method_name }

def some_meaningfull_method_name
  if foo?
    [false]
  else
    [true]
  end
end

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.

Cop Idea: Per attribute validations
3 participants