-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
fix: cache key stability #142
Conversation
5fa5161
to
9b4b3d7
Compare
1d5cfc6
to
d5a9bd4
Compare
Ensure consistency of main and post configuration by storing and restoring it from state, which in turn ensures cache key stability. Also: * Fixed some typos. * Use core.error for logging errors. * Fix inverted condition on cache-all-crates. Reverts: Swatinem#138 Fixes Swatinem#140
d5a9bd4
to
68377a4
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, except the Object.assign
/JSON.parse
roundtrips. We should make it explicit what we are serializing, and deserialize things manually as well.
Make the loading of config from state explicit by splitting into a dedicated static method.
@Swatinem to make sure you didn't miss the notification 😃 |
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, though the specifics of de/serialization can be improved later
Thanks, appreciate your time. |
Ensure consistency of main and post configuration by storing and restoring it from state, which in turn ensures cache key stability.
Also:
Reverts: #138
Fixes #140