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

Class Macros & Inheritance #39

Merged
merged 2 commits into from
Aug 12, 2021
Merged

Class Macros & Inheritance #39

merged 2 commits into from
Aug 12, 2021

Conversation

tstone
Copy link
Collaborator

@tstone tstone commented Aug 11, 2021

Background

This PR changes how the class macros (force_cache_class and cache_version) interact with inheritance.

Consider the following scenarios. What would the correct cache prefix be?

Case 1:

class Foo
  include AtomicCache::GlobalLMTCacheConcern
  cache_version(6)
end
class Bar < Foo
  force_cache_class('asdf')
end

Case 2:

class Foo
  include AtomicCache::GlobalLMTCacheConcern
  force_cache_class('asdf')
end
class Bar < Foo
  cache_version(6)
end

This PR changes is so that the preference of values is: Local override > Parent override > default value. Previously the gem would not consider parent override values. Summarized below as...

BEFORE:
Case 1 = asdf
Case 2 = bar:v6

AFTER:
Case 1 = asdf:v6
Case 2 = asdf:v6

Version

This change will not break the interface of existing code, but could possibly be considered backwards incompatible due to the key changing (assuming something relies on knowing what that value is). Since the gem is still in pre-release/pre-1.0 status though a minor version change will be used.

Tasks

  • Code of Conduct reviewed
  • Specs written and passing
  • Backwards-incompatible changes called out in this PR
  • Increment the version file (./lib/atomic_cache/version.rb) when applicable

@tstone tstone requested a review from a team August 11, 2021 21:55
@@ -29,3 +29,65 @@ class Foo < ActiveRecord::Base
cache_version(5)
end
```

### Inheritance
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥 This is a great write up.


it 'uses the prefers the forced class name from the macro' do
subject.expire_cache
expect(key_storage.store).to have_key(:'bar:v6:lmt')
Copy link
Contributor

@lebeerman lebeerman Aug 11, 2021

Choose a reason for hiding this comment

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

if no force_cache_class is used would the expectation be foo7:v6:lmt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if the force_cache_class on line 157 was absent, it would be the same. If the force_cache_class on line 160 was absent, it would be foo:v6:lmt, because it prefers explicit overrides on the parent ahead of default values on the class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, makes sense. Also, saw the notes in the new docs that both force methods should work in a similar way wrt inheritance.

Copy link
Contributor

@lebeerman lebeerman left a comment

Choose a reason for hiding this comment

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

😎 The improvement to the docs are awesome.

@tstone tstone merged commit 64d27ee into main Aug 12, 2021
@tstone tstone deleted the bugfix-class-macro-inheritance branch August 12, 2021 15:37
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.

3 participants