Rework Update JSON encoders and decoders
#3261
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3261 +/- ##
==========================================
+ Coverage 89.77% 89.81% +0.03%
==========================================
Files 171 171
Lines 4970 4989 +19
Branches 494 495 +1
==========================================
+ Hits 4462 4481 +19
Misses 508 508 ☔ 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. |
I tried this now here: https://github.com/scala-steward-org/scala-steward/compare/topic/rework-update-codecs-2 |
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.
fthomas
force-pushed
the
topic/rework-update-codecs
branch
from
026eb23 to
f362631
Compare
| Encoder.instance[ForArtifactId] { forArtifactId => | ||
| Json.obj( | ||
| "ForArtifactId" -> Json.obj( | ||
| "crossDependency" -> forArtifactId.crossDependency.asJson, | ||
| "newerVersions" -> forArtifactId.newerVersions.asJson, | ||
| "newerGroupId" -> forArtifactId.newerGroupId.asJson, | ||
| "newerArtifactId" -> forArtifactId.newerArtifactId.asJson | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
suggestion (non-blocking): You can also use Encoder/Decoder forProduct to reduce boilerplate from this file
This comment follows the conventionalcomments.org standard
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.