-
-
Notifications
You must be signed in to change notification settings - Fork 270
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/DotSeparatedKeys
cop
#325
Conversation
@koic Any interest in this? |
Wouldn't this better be part of rubocop-i18n: https://github.com/puppetlabs/rubocop-i18n |
Maybe. We have already merged another cop https://github.com/rubocop-hq/rubocop-rails/blob/master/lib/rubocop/cop/rails/short_i18n.rb I was thinking the same when noticed that gem existence, then checked it out - and or I do not understand the purpose of that gem (looking at its cops) or personally I find it useless and would prefer to have such cops as in this PR in this gem. |
RUBY | ||
end | ||
|
||
it 'does not register an offense when `scope` is not a string nor a symbol' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mention that "scope
is not a string". But `scope is an Array or a Sybmol in the examples.
Can you add an example for a string scope?
To my personal taste,
t :record_not_found, scope: 'activerecord.errors.messages'
reads better than
t scope: 'activerecord.errors.messages.record_not_found'
I'll gladly suggest an amendment to the Rails Style guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t :record_not_found, scope: 'activerecord.errors.messages'
will already be corrected to
t 'activerecord.errors.messages.record_not_found'
Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am I missing something?
Yes, an example for this case 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with that change and a new test case.
d473b74
to
d60f144
Compare
I'm sorry for the delay. Can you rebase this PR with the latest master branch? |
d60f144
to
7529f53
Compare
@koic Rebased. |
Thank you! |
An implementation of https://rails.rubystyle.guide/#dot-separated-keys