Skip to content

Commit 6069792

Browse files
committed
Fix: repo-config for pullRequests.draft should not be wiped when combined with _unset_ config
This fixes a bug that came in with the initial introduction of the `pullRequests.draft` config setting with this PR: * #3628 The bug was that when combining global & local repo config, the result of combining `pullRequests.draft = true` with a config with _no_ setting for `pullRequests.draft` would be that `pullRequests.draft` was _unset_ (ie given the value `None`, rather than `Some(true)`. This is because of the behaviour of `.mapN()`, which I hadn't really thought through: https://github.com/scala-steward-org/scala-steward/blob/373d5a70533a024389bb93f5028d0fee81a6cbd6/modules/core/src/main/scala/org/scalasteward/core/repoconfig/PullRequestsConfig.scala#L66 ...in retrospect, it's not too surprising that `.mapN()` on two `Option`s only produces a populated `Some` if _both_ the inputs are `Some` - which is not the behaviour we want here. For us, if only _one_ of the configs has a value set, we definitely want to use that value. In the end, the neatest way I could find to get the desired behaviour was to remove the thing stopping us from having a `Monoid[Option[Boolean]]` - ie that there is no _general_ `Monoid[Boolean]` (there is no _one_ correct way to combine two `Boolean`s). We can create a specific one for this case, that says `Boolean`s should be combined with logical-`OR` (rather than `AND`, for instance) - and only expose it in the very narrow scope of where we want to use it. * guardian/scala-steward-public-repos#102 (comment)
1 parent 7e9cf7f commit 6069792

File tree

2 files changed

+15
-1
lines changed

2 files changed

+15
-1
lines changed

modules/core/src/main/scala/org/scalasteward/core/repoconfig/PullRequestsConfig.scala

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,10 @@ object PullRequestsConfig {
6363
grouping = x.grouping |+| y.grouping,
6464
includeMatchedLabels = x.includeMatchedLabels.orElse(y.includeMatchedLabels),
6565
customLabels = x.customLabels |+| y.customLabels,
66-
draft = (x.draft, y.draft).mapN(_ || _)
66+
draft = {
67+
implicit val booleanOrMonoid: Monoid[Boolean] = Monoid.instance(false, _ || _)
68+
x.draft |+| y.draft
69+
}
6770
)
6871
)
6972
}
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,20 @@
11
package org.scalasteward.core.repoconfig
22

3+
import cats.implicits.*
34
import cats.kernel.laws.discipline.MonoidTests
45
import munit.DisciplineSuite
56
import org.scalasteward.core.TestInstances.*
67

78
class PullRequestsConfigTest extends DisciplineSuite {
89
checkAll("Monoid[PullRequestsConfig]", MonoidTests[PullRequestsConfig].monoid)
10+
11+
test("global config to use 'draft' PRs should be retained after merging with local config") {
12+
val draftTrue = PullRequestsConfig(draft = Some(true))
13+
val draftUnset = PullRequestsConfig()
14+
val draftFalse = PullRequestsConfig(draft = Some(false))
15+
16+
assert((draftTrue |+| draftUnset).draft === Some(true))
17+
assert((draftTrue |+| draftFalse).draft === Some(true))
18+
assert((draftUnset |+| draftUnset).draft === None)
19+
}
920
}

0 commit comments

Comments
 (0)