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/I18nLocaleTexts cop #643

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

fatkodima
Copy link
Contributor

Closes #624

cc @pirj

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Fantastic, thank you (for my dreams come true)!

def on_send(node)
case node.method_name
when :validates
validation_message(node).each do |text_node|
Copy link
Member

Choose a reason for hiding this comment

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

Cosmetic: is .each necessary? Would just validation_message(node) do |text_node| work?
I understand that def_node_search needs to be replaced with def_node_matcher, and the node pattern gains more weight.

I'm thinking of something like (please accept my apologies for a contrived example):

  validates :username,
    uniqueness: {
      message: ->(object, data) do
        {message: 'ough oh', value: data.value}.to_s
      end
    }

Copy link
Member

Choose a reason for hiding this comment

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

Writing this as:

def on_send(node)
  validation_message?(node, &method(:add_offence))
  redirect_to_flash?(node, &method(:add_offence))
  flash_assignment?(node, &method(:add_offence))
  mail_subject?(node, &method(:add_offence))
end

would also allow getting rid of an early return and to_a.last.

Subjectively, cleaner. But again, pretty cosmetic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cosmetic: is .each necessary?

Checked, indeed works without it.

Agreed, cleaner, despite I personally not a fan of &method construction.
But the possible problem with this is that all the lines are executed, even if we have validation and it should skip the rest of the checks.

So, should be done as return if something?(...) x 4 ?

# # code_format: "only letters"
#
# class Product < ApplicationRecord
# validates :code, format: { with: /\A[a-zA-Z]+\z/, message: :code_format }
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find a mention of this in Rails guides or API. Does message: :code_format work?
Do you think it would be possible to come up with a more conventional example like:

validates :name, presence: true

The key for the error message in this case is :blank. Active Record will look up this key in the namespaces ... activerecord.errors.models.[model_name].attributes.[attribute_name], ... it will try the following keys:

activerecord.errors.models.user.attributes.name.blank

This way, message: doesn't even have to be specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 16 to 21
# # en:
# # activerecord:
# # errors:
# # models:
# # product:
# # code_format: "only letters"
Copy link
Member

Choose a reason for hiding this comment

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

Cosmetic: does this need to be double commented? YAML is a file, too, just like Ruby code below. Is there a specific semantic to it, will it appear differently in cop's docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this is required, because otherwise documentation generator raises an error regarding colons.

lib/rubocop/cop/rails/i18n_locale_texts.rb: Syntax Error in an example. unexpected token tCOLON

RESTRICT_ON_SEND = %i[validates redirect_to []= mail].freeze

def_node_search :validation_message, <<~PATTERN
(pair (sym :message) $str_type?)
Copy link
Member

Choose a reason for hiding this comment

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

Would $str work the same way as $str_type??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works, thanks 👍

@pirj
Copy link
Member

pirj commented Feb 22, 2022

Wondering what it would stumble across real-world-rails apps 🤔

@fatkodima
Copy link
Contributor Author

Ran on Gitlab's codebase:

24875 files inspected, 87 offenses detected

and 0 errors.

# end
# end
#
# class UserMailer < ApplicationMailer
Copy link
Member

@koic koic Feb 23, 2022

Choose a reason for hiding this comment

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

Is #bad forgotten?

Suggested change
# class UserMailer < ApplicationMailer
# # bad
# class UserMailer < ApplicationMailer

@koic
Copy link
Member

koic commented Mar 4, 2022

@fatkodima Looks good overall. Can you fix the one left comment (#643 (comment)) and squash your commits into one?

@fatkodima fatkodima force-pushed the i18n_locale_texts branch from b2f97a9 to 4ce004d Compare March 4, 2022 13:05
@fatkodima
Copy link
Contributor Author

@koic Done.

@koic koic merged commit c4d242d into rubocop:master Mar 4, 2022
@koic
Copy link
Member

koic commented Mar 4, 2022

Thank you!

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.

Add a cop to enforce internationalization
3 participants