Add request_reviews and update Mergify actions #320
Conversation
|
Not sure if I targeted the right branch for this PR—feel free to retarget if necessary. |
|
Thanks! You may want to retarget to |
|
I'll rebase and push it up for |
mergify/src/main/scala/org/typelevel/sbt/mergify/MergifyAction.scala
Outdated
Show resolved
Hide resolved
| @@ -57,6 +59,21 @@ object MergifyAction { | |||
| } | |||
| } | |||
|
|
|||
| final case class RequestReviews(users: List[String] = Nil) extends MergifyAction { | |||
There was a problem hiding this comment.
Can we also add the teams and users_from_teams keys?. Also frustratingly seems like all of these can be either a List or a Map. I wonder if we should just go with the Map since it subsumes List and maybe add some helpful constructors.
https://docs.mergify.com/actions/request_reviews/
I know it's annoying, but fixing these binary-compatibly will be even more annoying :)
There was a problem hiding this comment.
Good idea, I'll play with this tomorrow or later this weekend.
There was a problem hiding this comment.
Had some extra time tonight after all. WDYT of what I just pushed up?
(I wondered whether any of these should be case classes if we're worried about bincompat, btw. Should I change the others too?)
There was a problem hiding this comment.
Yeah, case classes are a usability vs maintainability trade-off. I think it's okay to use them here, as long as we do our best to include all the members right from the start. Bincompat is not a huge deal for an sbt plugin, but I still like to avoid breaking it if I can.
mergify/src/main/scala/org/typelevel/sbt/mergify/MergifyAction.scala
Outdated
Show resolved
Hide resolved
| @@ -59,16 +61,30 @@ object MergifyAction { | |||
| } | |||
| } | |||
|
|
|||
| final case class RequestReviews(users: List[String] = Nil) extends MergifyAction { | |||
| final class RequestReviews(val users: Either[Seq[String], Map[String, Int]], val randomCount: Option[Int]) extends MergifyAction { | |||
There was a problem hiding this comment.
Rather than Either I wonder if we should just use a Map, since a List is equivalent to a Map with all weights the same e.g. 1. Also I can't remember how Either renders by default with Circe?
The generated config will be a bit weird, but it's technically not user-facing.
There was a problem hiding this comment.
Also I can't remember how Either renders by default with Circe
I folded them together so that if it's a list it gets rendered without the weights—I was thinking it would be kind of confusing to have weights if they're not specified. I know it's not something users should be editing, but it's still visible and people could be spot-checking it or copy/pasting it into the Mergify config editor for validation or to see what rules would be triggered on different PRs.
There was a problem hiding this comment.
Ok, fair point. Let's keep it then. How does Circe generate yaml from Either?
Actually, you can add a RequestReviews action to the sbt-typelevel mergify config in build.sbt so we can dog-food this :)
9cf4e4a to
2d06c9b
Compare
Most of our OSS projects use the
request_reviewsandupdateMergify actions, which aren't supported yet. e.g. https://github.com/Dwolla/fs2-pgp/blob/main/.mergify.ymlWe have also typically used merge queues instead of merging directly, but I'm not sure how important that is. I remember we switched from "strict" merges when Mergify deprecated them, but maybe that was overkill for our needs. I'll play with it and see; we can always add that in a future PR.