Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make all values in config classes optional #3515

Merged
merged 19 commits into from
Jan 4, 2025
Merged

Conversation

fthomas
Copy link
Member

@fthomas fthomas commented Jan 2, 2025

This changes RepoConfig and the other *Config classes such that all values are optional and there are no default parameter values except Nones. This allows us to get rid of circe-generic-extras' Configuration.default.withDefaults and deriveConfiguredCodec since missing Option values do not cause an error during decoding. So {} can still be decoded into a RepoConfig, and there are a lot of tests in RepoConfigAlgTest that ensure that sparse configurations with a lot of missing values can still be decoded.

Another benefit of this change is that RepoConfig.show can now use deepDropNullValues so that logged RepoConfigs only include stuff which was actually set by a user. For example, before this change, logging a RepoConfig was similar to this:

INFO  Parsed repo config {
   "commits" : {
     "message" : null
   },
   "pullRequests" : {
     "frequency" : null,
     "grouping" : [
     ],
     "includeMatchedLabels" : null,
     "customLabels" : [
     ]
   },
   "scalafmt" : {
     "runAfterUpgrading" : null
   },
   "updates" : {
     "pin" : [
     ],
     "allow" : [
     ],
     "allowPreReleases" : [
     ],
     "ignore" : [
     ],
     "limit" : null,
     "fileExtensions" : null
   },
   "postUpdateHooks" : null,
   "updatePullRequests" : "always",
   "buildRoots" : null,
   "assignees" : [
   ],
   "reviewers" : [
   ],
   "dependencyOverrides" : [
   ]
 }

The same RepoConfig is now logged as:

INFO  Parsed repo config {
   "updatePullRequests" : "always"
 }

Before merging this PR, I'll squash the commits and provide a better commit message, but for potential reviewers, it should be easier to look at each commit individually.

This continues the work started in #3514 of getting rid of circe-generic-extras so that Scala Steward can be built with Scala 3.

Copy link

codecov bot commented Jan 2, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.63%. Comparing base (12bcad7) to head (fc71579).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...ala/org/scalasteward/core/nurture/NurtureAlg.scala 50.00% 2 Missing ⚠️
...lasteward/core/repoconfig/PullRequestsConfig.scala 66.66% 1 Missing ⚠️
...g/scalasteward/core/repoconfig/UpdatesConfig.scala 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3515      +/-   ##
==========================================
- Coverage   89.64%   89.63%   -0.02%     
==========================================
  Files         171      171              
  Lines        3525     3540      +15     
  Branches      315      331      +16     
==========================================
+ Hits         3160     3173      +13     
- Misses        365      367       +2     

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

pullRequests: PullRequestsConfig = PullRequestsConfig(),
scalafmt: ScalafmtConfig = ScalafmtConfig(),
updates: UpdatesConfig = UpdatesConfig(),
commits: Option[CommitsConfig] = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): Do we still need the None here? Is it just for tests?

This comment follows the conventionalcomments.org standard

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is just for the tests. RepoConfigAlgTest would be cluttered with Nones for example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes total sense. Thanks!

@fthomas
Copy link
Member Author

fthomas commented Jan 3, 2025

This change maybe also enables us to remove nonExistingUpdatePattern since we now can differentiate between the default value and an empty list that was set by a user.

Copy link
Member

@mzuehlke mzuehlke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a very systematic change 👍
Do you think converting the fields of the the case classes into private val would be a good idea to prevent accidental accessing them and forgetting the default value ?

@fthomas
Copy link
Member Author

fthomas commented Jan 4, 2025

Do you think converting the fields of the the case classes into private val would be a good idea to prevent accidental accessing them and forgetting the default value ?

Sounds like a good idea to me. I've done that in fc71579.

@fthomas fthomas merged commit a31586b into main Jan 4, 2025
12 checks passed
@fthomas fthomas deleted the topic/config-null-values-2 branch January 4, 2025 16:25
fthomas added a commit that referenced this pull request Jan 4, 2025
This concludes the work started in #3514 and #3515 to remove the
dependency on circe-generic-extras.

Here are some notes about the changes:
- `Resolver` cases had non-optional fields with defaults that needed to
  be made optional so that the derived decoder does not fail if those
  are not provided
- `ArtifactChange` and `ArtifactChanges` could have always used `deriveDecoder`
  since the case classes have no defaults
- `ScalafixMigrations` had a default value but a `scalafix-migrations.conf`
  without a `migrations` fields makes no sense to me and it should be the same
  as `ArtifactChanges` anyway
fthomas added a commit that referenced this pull request Jan 16, 2025
I forgot to update this reference in #3515. This is a subtle change in semantics. If `allow == Some(Nil)`  `allow.isEmpty` is `false` and `allowOrDefault.isEmpty` is `true`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants