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/DotSeparatedKeys cop #325

Merged
merged 1 commit into from Jun 6, 2022

Conversation

fatkodima
Copy link
Contributor

@fatkodima
Copy link
Contributor Author

@koic Any interest in this?

@mobilutz
Copy link
Contributor

Wouldn't this better be part of rubocop-i18n: https://github.com/puppetlabs/rubocop-i18n

@fatkodima
Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

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 😄

Copy link
Contributor Author

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.

@koic
Copy link
Member

koic commented Jun 6, 2022

I'm sorry for the delay. Can you rebase this PR with the latest master branch?

@fatkodima
Copy link
Contributor Author

@koic Rebased.

@koic koic merged commit 6d82181 into rubocop:master Jun 6, 2022
@koic
Copy link
Member

koic commented Jun 6, 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.

None yet

4 participants