-
Notifications
You must be signed in to change notification settings - Fork 512
Description
this is a fairly minor issue, basically distracting/unclear to newcomer developers exploring the Scala Steward codebase
At the moment, Update.ForGroupId() takes a constructor parameter of multiple Update.ForArtifactIds, but within Update.ForGroupId there's nothing enforcing that all the Update.ForArtifactId's have the same Maven group-id:
scala-steward/modules/core/src/main/scala/org/scalasteward/core/data/Update.scala
Lines 118 to 120 in d7330f1
| final case class ForGroupId( | |
| forArtifactIds: Nel[ForArtifactId] | |
| ) extends Single { |
Due to the way Update.ForGroupIds are constructed, they will have the same Maven group-id:
scala-steward/modules/core/src/main/scala/org/scalasteward/core/data/Update.scala
Lines 174 to 181 in d7330f1
| def groupByGroupId(updates: List[ForArtifactId]): List[Single] = { | |
| val groups0 = | |
| updates.groupByNel(s => (s.groupId, s.currentVersion, s.newerVersions)) | |
| val groups1 = groups0.values.map { group => | |
| if (group.tail.isEmpty) group.head else ForGroupId(group) | |
| } | |
| groups1.toList.distinct.sorted | |
| } |
...but Update.ForGroupId itself doesn't guarantee it.
Within the class there are also parts like this, where we just assume the first group-id is the same for everything:
scala-steward/modules/core/src/main/scala/org/scalasteward/core/data/Update.scala
Lines 127 to 128 in 63a0476
| override def groupId: GroupId = | |
| dependencies.head.groupId |
Enforcement
- Most easy: adding a
require()statement insideUpdate.ForGroupId() - Trickier, but more type-worthy: change the constructor parameter so that it no longer takes
Update.ForArtifactIds - instead, it allows just single fields for group-id, versions, etc, and a list of something - possiblyArtfactIds orCrossDependencys - that stores those multiple artifact ids. Note thatUpdateinstances are persisted, so changing the format of them will require updating the JSON decoders to be tolerant of both the old and new format.
Additional context
Here's a sketchy sketch of the Update type hierarchy we made while discussing this!
