Skip to content

Conversation

rtyley
Copy link
Contributor

@rtyley rtyley commented Oct 1, 2025

This fixes a bug that came in with the initial introduction of the pullRequests.draft config setting:

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).

Here's an example of where this was a problem in the wild:

Cause

This arose because of the behaviour of .mapN(), which I hadn't really thought through:

...in retrospect, it's not too surprising that .mapN() on two Options 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.

Fix

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 Booleans). We can create a specific one for this case, that says Booleans 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.

@rtyley rtyley force-pushed the global-config-of-draft-should-be-retained branch from 2ebacf1 to 6a1b4e6 Compare October 1, 2025 16:21
Copy link

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.77%. Comparing base (7e9cf7f) to head (6069792).
⚠️ Report is 24 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3719   +/-   ##
=======================================
  Coverage   89.77%   89.77%           
=======================================
  Files         174      174           
  Lines        5054     5055    +1     
  Branches      489      492    +3     
=======================================
+ Hits         4537     4538    +1     
  Misses        517      517           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rtyley rtyley force-pushed the global-config-of-draft-should-be-retained branch 4 times, most recently from 506331e to b9f21a1 Compare October 2, 2025 14:36
@rtyley rtyley changed the title Test that global config to use 'draft' PRs should be retained after merging with local config Fix: repo-config for pullRequests.draft should not be wiped when combined with _unset_ config Oct 2, 2025
@rtyley rtyley marked this pull request as ready for review October 2, 2025 14:45
@rtyley rtyley force-pushed the global-config-of-draft-should-be-retained branch from b9f21a1 to 54d8318 Compare October 2, 2025 14:48
…mbined 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)
@rtyley rtyley force-pushed the global-config-of-draft-should-be-retained branch from 54d8318 to 6069792 Compare October 2, 2025 16:40
@rtyley rtyley merged commit 2a028a1 into main Oct 2, 2025
18 checks passed
@mzuehlke mzuehlke added the bug Something isn't working label Oct 10, 2025
@mzuehlke mzuehlke added this to the 0.36.0 milestone Oct 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants