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/HashLiteralKeysConversion cop #1333

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fatkodima
Copy link
Contributor

Closes #1332.

@vlad-pisanov
Copy link
Contributor

vlad-pisanov commented Aug 18, 2024

Nice! Although I'm seeing this false positive:

value = { 'foo' => 'bar' }
{ key: value }.deep_symbolize_keys

For deep_* methods, we'd need to check that all nested values are literals.

@fatkodima fatkodima force-pushed the hash_literal_keys_conversion branch from 661d89b to 54cfc83 Compare August 18, 2024 23:16
@fatkodima
Copy link
Contributor Author

Good catch! Thank you.

@vlad-pisanov
Copy link
Contributor

Another tricky false positive: the deep_* methods (surprisingly) recursively drill into Array values, so this cop would need to do the same.

{ a: [ { 'b' => 1 } ] }.deep_symbolize_keys
# => { a: [ { b: 1 } ] }

Lastly, if we wanted to be really fancy, we could also detect interpolated keys: ("name_#{index}" => ...", :"name_#{index}" => ...) and explicitly converted keys (foo.to_sym => ..., foo.to_s => ...) but that might be a stretch. 😛

@fatkodima fatkodima force-pushed the hash_literal_keys_conversion branch from 54cfc83 to df568b2 Compare August 19, 2024 09:33
@fatkodima
Copy link
Contributor Author

Fixed for nested arrays and deep_ methods 👍.

Lastly, if we wanted to be really fancy, we could also detect interpolated keys

Do not want to be fancy - the code is already pretty complex and the usecase is pretty rare, imo.

@vlad-pisanov
Copy link
Contributor

Last false positive in our repo: a combination of the previous two cases (nested arrays + non-literal hash value)

value = { 'foo' => 'bar' }
{ a: [{ value: value }]  }.deep_symbolize_keys # ⚠️ Redundant hash keys conversion, all the keys have the required type.
# => { a: [{ foo: 'bar' }] }

@fatkodima fatkodima force-pushed the hash_literal_keys_conversion branch from df568b2 to b0441e3 Compare August 19, 2024 14:08
@fatkodima
Copy link
Contributor Author

Fixed that too 😅

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.

Detect unnecessary stringify_keys/symbolize_keys calls
2 participants