Rework Update JSON encoders and decoders
#3261
Conversation
This is an attempt to clarify the various `Encoder` and `Decoder` instances that make up `Encoder[Update]` and `Decoder[Update]` that are used by the `PullRequestRepository` for writing/reading updates to/from disk. I had a hard time understanding what was going on because * nearly all of the instances are implicit (for derivation) but only the instances for `Update` are used by other parts of the program * semi-derivation was mixed with manual modifications * decoders for compatibility with old caches depend on derivation and are defined ad-hoc In this change I tried to bring more clarity to these instances by * giving compatibility decoders their own name * forgoing semi-derivation and define all instances manually. IMHO this makes it easier to evolve them if there are compatibility constraints because of old caches * making every instance except `Encoder[Update]` and `Decoder[Update]` private and non-implicit * only testing them via the public `Encoder[Update]` and `Decoder[Update]` instances This might be a controversial change and I ackknowledge that one gains more understanding by doing this rework. I'm therefore biased that these changes actually clarify anything. I'm fine with letting this age for a while before merging to see if I still feel this is an improvement then.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3261 +/- ##
==========================================
+ Coverage 91.18% 91.25% +0.06%
==========================================
Files 167 167
Lines 3405 3432 +27
Branches 306 304 -2
==========================================
+ Hits 3105 3132 +27
Misses 300 300 ☔ View full report in Codecov by Sentry. |
|
@fthomas do we still need to keep compatibility for old caches? wouldn't have those already expire and one with the new encoder be created? |
This is true for caches that have been updated since #2898 (which was the last change to the On the other hand it is very tempting to just semi-derive all our instances again and remove the compatibility decoders. It would simplify that part of the code base as much as possible. |
That makes sense. Thanks for the clarification 😊 |
There was a problem hiding this comment.
This brings consistency to all the different version 👍
The other alternative would be to have matching case classes for all different versions, then use semiauto and convert afterwards into the current version.
But I have the feeling that your version is easier to understand.
That sounds interesting! I'll give it a try. |
This is an attempt to clarify the various
EncoderandDecoderinstances that make upEncoder[Update]andDecoder[Update]that are used by thePullRequestRepositoryfor writing/reading updates to/from disk.I had a hard time understanding what was going on because
Updateare used by other parts of the programIn this change I tried to bring more clarity to these instances by
Encoder[Update]andDecoder[Update]private and non-implicitEncoder[Update]andDecoder[Update]instancesThis might be a controversial change and I ackknowledge that one gains more understanding by doing this rework. I'm therefore biased that these changes actually clarify anything. I'm fine with letting this age for a while before merging to see if I still feel this is an improvement then.