-
Notifications
You must be signed in to change notification settings - Fork 17
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
Rails 7.0 support and drop older railses #63
base: main
Are you sure you want to change the base?
Conversation
What happens if someone report a bug with Rails 6.1 with the actual released branch? We create a release branch for the current major? |
We can maintain a |
1a157d5
to
cdf5262
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.
LGTM
@serializer = serializer | ||
@compressor = compressor | ||
end | ||
attr_accessor :swallow_exceptions |
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.
We should remove it since we don't support it.
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.
Good catch.
cdf5262
to
e4e8b2c
Compare
This LGTM but I haven't been able to fully follow the corresponding Rails changes, so I'm holding off reviewing yet. I'll focus on decoupling our code from this gem altogether where it makes sense so changes here have less impact. |
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'm just commenting for now, I'll try creating a failing test to check what I commented.
Entry.new(payload, compress: false) | ||
end | ||
else | ||
super(payload) |
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 this will break raw values read back from the cache.
Currently a raw value written to the cache with Rails.cache.write("foo", "bar", raw: true)
can be read back with just Rails.cache.read("foo")
(no raw: true
), but with this change this won't work unless you pass raw: true
.
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.
Yes, I broke this knowingly.
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.
This fails:
def test_raw
assert @cache.write("foo", "1", raw: true)
assert_equal "1", @cache.read("foo")
end
It seems like all tests on raw values now pass raw: true
to read
/ fetch
, so maybe this is intentional? But if so it will break a lot of stuff, and I'm not even sure how it could be handled in contexts where you're not sure if the stored value is raw or not.
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.
Approving since the breaking change is known, although I'm not sure how it should be handled.
You think? Shouldn't be too hard to track down no? |
2f73343
to
1cb8312
Compare
3ed14ed
to
402f581
Compare
As well as remove unused features (e.g. read only).
This if merged should be a major bump.