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 Rails/EnumKeywordArgs cop #1239

Closed

Conversation

maxprokopiev
Copy link
Contributor

@maxprokopiev maxprokopiev commented Feb 16, 2024

This PR adds a new cop called Rails/EnumKeywordArgs.

This cop checks for the use of deprecated keyword arguments in enums. See more in #1238

Style guide PR: rubocop/rails-style-guide#356


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

Defining enums with keyword arguments is deprecated
and will be removed in Rails 7.3.
@maxprokopiev maxprokopiev marked this pull request as ready for review February 16, 2024 11:04
@maxprokopiev
Copy link
Contributor Author

@ghiculescu
Copy link
Contributor

see rubocop/rails-style-guide#356 (comment)

A linter would still be helpful if it helps people autocorrect so they can migrate to the new API automatically.

@maxprokopiev
Copy link
Contributor Author

reopening this one since there seem to be some demand to have a linter, but not an entry to the style guide

@maxprokopiev maxprokopiev reopened this Feb 28, 2024
@fatkodima
Copy link
Contributor

Rails 7.2 is about to be released (there is a stable branch already https://github.com/rails/rails/tree/7-2-stable).

@koic Can you please consider this PR?

@chaadow
Copy link

chaadow commented May 27, 2024

@maxprokopiev Hi, btw i've used your branch to upgrade a legacy rails app that uses this old format of keyword args. your implementation detected pretty much all my cases except these 2 cases :

`-

SOME_HASH_CONSTANT = { 0: 'active', 1: 'inactive }

enum my_enum: SOME_HASH_CONSTANT

==> the cop does not detect this at all. But since I have deprecations raise in development, i was able to eager load ( by calling bin/rails zeitwerk:check and find these edge cases.

2- Second point i noticed, the autocorrection is not available for _suffix and _prefix

# Here the hash is correct, your cop detects the use of "undierscore
# but the autocorrection is not available to change `_suffix` to `suffix
enum :my_enum,  { 0: 'active', 1: 'inactive }, _suffix: true

for my case, i think it was : enum :my_enum, SOME_HASH_CONSTANT, _suffix: true

@ohbarye
Copy link
Contributor

ohbarye commented Aug 10, 2024

Now that Rails 7.2.0 has been released, I hope this pull request moves forward for a smooth upgrade. 👍

https://rubyonrails.org/2024/8/10/Rails-7-2-0-has-been-released

# # good
# enum :status, { active: 0, archived: 1 }, prefix: true
#
class EnumKeywordArgs < Base
Copy link
Member

Choose a reason for hiding this comment

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

How about the name Rails/EnumSyntax?

Suggested change
class EnumKeywordArgs < Base
class EnumSyntax < Base

# Looks for enums written with keyword arguments.
#
# Defining enums with keyword arguments is deprecated
# and will be removed in Rails 7.3.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# and will be removed in Rails 7.3.
# and will be removed in Rails 8.0.

Comment on lines +429 to +430
Enabled: pending
VersionAdded: '<<next>>'
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the Severity: warning setting:

Suggested change
Enabled: pending
VersionAdded: '<<next>>'
Enabled: pending
Severity: warning
VersionAdded: '<<next>>'

# enum :status, { active: 0, archived: 1 }, prefix: true
#
class EnumKeywordArgs < Base
extend AutoCorrector
Copy link
Member

Choose a reason for hiding this comment

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

I think that it has been supported since Rails 5.0:

Suggested change
extend AutoCorrector
extend AutoCorrector
extend TargetRailsVersion
minimum_target_rails_version 5.0

@@ -423,6 +423,14 @@ Rails/EnumHash:
Include:
- app/models/**/*.rb

Rails/EnumKeywordArgs:
Description: 'Use positional arguments over keyword arguments when defining enums.'
StyleGuide: 'https://rails.rubystyle.guide#enums'
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove it because it follows a different style guide rule?

Suggested change
StyleGuide: 'https://rails.rubystyle.guide#enums'

@@ -0,0 +1 @@
* [#1239](https://github.com/rubocop/rubocop-rails/pull/1239): Add new `Rails/EnumKeywordArgs`. ([@maxprokopiev][])
Copy link
Member

@koic koic Aug 19, 2024

Choose a reason for hiding this comment

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

Suggested change
* [#1239](https://github.com/rubocop/rubocop-rails/pull/1239): Add new `Rails/EnumKeywordArgs`. ([@maxprokopiev][])
* [#1238](https://github.com/rubocop/rubocop-rails/issues/1238): Add new `Rails/EnumSyntax`. ([@maxprokopiev][])

@koic
Copy link
Member

koic commented Aug 19, 2024

@maxprokopiev ping.

@koic
Copy link
Member

koic commented Aug 22, 2024

I opened #1336 based on your commit to include it in the upcoming new version planned for release soon. Thank you.

@koic koic closed this Aug 22, 2024
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.

6 participants