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

Enable Style/RedundantFetchBlock and autocorrect-all #47327

Closed
wants to merge 1 commit into from

Conversation

zzak
Copy link
Member

@zzak zzak commented Feb 9, 2023

As @natematykiewicz pointed out in #47307 there is a rubocop rule for this.

@@ -280,7 +280,7 @@ def define_default_attribute(name, value, type, from_user:)
name,
value,
type,
_default_attributes.fetch(name.to_s) { nil },
_default_attributes.fetch(name.to_s, nil),
Copy link
Contributor

Choose a reason for hiding this comment

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

If nil is the default, you can just use [].

Suggested change
_default_attributes.fetch(name.to_s, nil),
_default_attributes[name.to_s],

Copy link
Member Author

Choose a reason for hiding this comment

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

@natematykiewicz Thank you, but I don't think we should change this yet unless there is a specific rule we can apply that supports it. I'd like to avoid making cosmetic changes without a "failing" case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you closed it because there's not a good way to differentiate Rails.cache from Hash. Makes sense. It's still possible this specific line using [] would be a performance change and not a cosmetic one. But you'd need a benchmark to see if it's actually faster.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is solving a different problem. Feel free to open a PR with benchmarks 🙏

@zzak
Copy link
Member Author

zzak commented Feb 9, 2023

There's a bunch of related failures that should be fixed too:

ActiveSupport::Cache::RedisCacheStoreTests::FailureSafetyFromUnavailableClientTest#test_fetch_read_failure_does_not_attempt_to_write:
--
  | TypeError: no implicit conversion of Symbol into Integer
  | /rails/activesupport/lib/active_support/cache.rb:380:in `[]'
  | /rails/activesupport/lib/active_support/cache.rb:380:in `fetch'
  | /rails/activesupport/test/cache/behaviors/failure_safety_behavior.rb:19:in `block in test_fetch_read_failure_does_not_attempt_to_write'
  | /rails/activesupport/test/cache/stores/redis_cache_store_test.rb:370:in `emulating_unavailability'

Probably this change was too aggressive 🤔

@zzak
Copy link
Member Author

zzak commented Feb 14, 2023

Ok, TIL the reason for all of the Active Support failures is because it's calling ActiveSupport::Cache::Store#fetch which is completely different: https://edgeapi.rubyonrails.org/classes/ActiveSupport/Cache/Store.html#method-i-fetch

After looking into this further it looks like this was already discussed on rubocop/rubocop#8196, and a previous attempt was made in #39765.

I think because we're required to rubocop:disable, until there is an alternative we cannot enable this rule.

@zzak zzak closed this Feb 14, 2023
@zzak zzak deleted the Style/RedundantFetchBlock branch February 14, 2023 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants