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

Rails/RedundantPresenceValidationOnBelongsTo detect an offence on validations with custom messages. #620

Closed
desheikh opened this issue Jan 11, 2022 · 6 comments · Fixed by #625
Labels
bug Something isn't working

Comments

@desheikh
Copy link

desheikh commented Jan 11, 2022

Rails/RedundantPresenceValidationOnBelongsTo detect and offence with a custom message attribute.


Expected behavior

validates :user presence: {
    message: 'My custom error message',
 }

rubocop should not raise an offense.

Actual behavior

rubocop raises an offense and will remove the validation on autocorrect.

Steps to reproduce the problem

Add a validation block to a model as above and run rubocop.

RuboCop version

$ [bundle exec] rubocop -V
1.24.1 (using Parser 3.1.0.0, rubocop-ast 1.15.1, running on ruby 2.7.5 x86_64-darwin19)
  - rubocop-rails 2.13.1
  - rubocop-rspec 2.7.0
@koic koic added the bug Something isn't working label Jan 11, 2022
@pirj
Copy link
Member

pirj commented Jan 11, 2022

I believe that the correct way of setting the message is via I18n files. E.g. for

class Foo < ApplicationRecord
  belongs_to :bar
  validates :bar, presence: { message: 'show me the way to the next whiskey bar' }
end

The convention is to:

# config/locales/en.yml (or config/locales/activerecord/en.yml depending on your preference)
en:
  activerecord:
    errors:
      models:
        foo:
          attributes:
            name:
              required: show me the way to the next whiskey bar

I don't particularly agree that the cop should ignore such validation.

We've marked the cop as unsafe for autocorrection, see #615, specifically because it affects the wording of the error message.

Would you like to send a PR with a clarification regarding error message I18n for @safety section?

@koic
Copy link
Member

koic commented Jan 12, 2022

I think this issue can be divided as follows:

  • Rails/RedundantPresenceValidationOnBelongsTo cop does not register an offense when using presence: { message: 'message' } option.
  • Another cop registers an offense to replace the option with I18n for that case.

@desheikh
Copy link
Author

Pirj, I wonder how common that is for rails apps unless they are localizing.
I like Koic's approach, as it separates the two concerns into individual rules.

@pirj
Copy link
Member

pirj commented Jan 12, 2022

Sounds reasonable 👍

Let's split the job, I can take care of the I18n cop. Would you send a PR with RedundantPresenceValidationOnBelongsTo changes?

@koic
Copy link
Member

koic commented Jan 12, 2022

Rails/RedundantPresenceValidationOnBelongsTo cop does not register an offense when using presence: { message: 'message' } option.

I will take it because there are #616 adjustments.

@pirj
Copy link
Member

pirj commented Jan 12, 2022

how common that is for rails apps unless they are localizing

Should be pretty common: https://rails.rubystyle.guide/#locale-texts

koic added a commit to koic/rubocop-rails that referenced this issue Jan 12, 2022
…lidationOnBelongsTo`

Fixes rubocop#620.

This PR fixes a false positive for `Rails/RedundantPresenceValidationOnBelongsTo`
using presence with a message and reverts rubocop#616 because accept the use case.
koic added a commit to koic/rubocop-rails that referenced this issue Jan 12, 2022
…lidationOnBelongsTo`

Fixes rubocop#620.

This PR fixes a false positive for `Rails/RedundantPresenceValidationOnBelongsTo`
using presence with a message.
@koic koic closed this as completed in #625 Jan 13, 2022
koic added a commit that referenced this issue Jan 13, 2022
…ndant_presence_validation_on_belongs_to

[Fix #620] Fix a false positive for `Rails/RedundantPresenceValidationOnBelongsTo`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants