-
-
Notifications
You must be signed in to change notification settings - Fork 263
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
Support Rails 7 syntax for Rails/EnumHash cop #1309
Support Rails 7 syntax for Rails/EnumHash cop #1309
Conversation
e0f53da
to
c6b9503
Compare
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.
📝 diff:
- Wrap existing tests with
context 'when old syntax is used'
. - Duplicate existing tests, wrap them with
context 'when Rails 7 syntax is used'
, and update to use the new syntax.- Remove
context 'with multiple enum definition for a enum method call'
as the new syntax doesn't support multiple enum definitions.
- Remove
expect_offense(<<~RUBY) | ||
enum :status, %i[active archived] | ||
^^^^^^^^^^^^^^^^^^^ Enum defined as an array found in `status` enum declaration. Use hash syntax instead. | ||
RUBY |
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.
Can you follow up expect_correction
with expect_offense
? Please do this for the diffs in this PR.
context 'when array syntax is used' do | ||
context 'with %i[] syntax' do | ||
it 'registers an offense' do | ||
context 'when Rails 7 syntax is used' 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.
I think there are generally no pragmatic issues, but it would be best if this new syntax is only detected in Rails 7.0 and later. In other words, it will not be detected in Rails 6.1 or earlier.
05b70bb
to
78c6796
Compare
Thank you for the review. I have updated the PR according to the comments. |
context 'when array syntax is used' do | ||
context 'with %i[] syntax' do | ||
it 'registers an offense' do | ||
context 'when Rails 7 syntax is used', :rails70 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.
Can you add a case that ensures offenses are not detected when using :rails61
context while still maintaining this :rails70
context? A representative test case could used expect_no_offenses
for the example code from L5 to L17.
e.g.,
context 'when Rails 6.1 or lower', :rails61 do
context 'when array syntax is used' do
context 'with %i[] syntax' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY)
enum :status, %i[active archived]
RUBY
end
end
end
end
@ytjmt ping. |
It is planned to be included in the release scheduled for this weekend. Let's add the specs later. |
Rails 7.0 introduced a new enum syntax ( rails/rails#41328 ):
Rails/EnumHash cop does't support the new syntax yet. This PR adds the new syntax support.
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.