feat: add the [approval_retention] config section - #183
Conversation
Adds the config surface the retention rules will hang off. No behaviour change: nothing reads these flags yet. The individual flags are *bool rather than bool so that unset can be told apart from an explicit false. That is what lets the umbrella work in both directions: on with nothing set turns every following flag on, and on with one flag set to false turns everything except that one on. string_literals, renames and fetch_orphaned_approval are opt-in and never follow the umbrella. The first two can alter behaviour without changing the shape of the code an approver reviewed; the third reaches the network. An end-to-end test pins the inertness claim: a config with no section and one spelling the section out with everything off produce the same bytes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the approval_retention configuration feature, allowing users to specify which kinds of changes (such as whitespace, comments, formatting, string literals, or renames) can retain an existing approval instead of dismissing it. The changes include documentation in the README, config parsing logic, and comprehensive unit tests. The reviewer suggested a refactoring opportunity to simplify the repetitive ...Enabled helper methods in internal/config/config.go by consolidating them into a single parameterized helper function.
| func (r *ApprovalRetention) WhitespaceEnabled() bool { | ||
| if r == nil { | ||
| return false | ||
| } | ||
| return r.defaultOn(r.Whitespace) | ||
| } | ||
|
|
||
| func (r *ApprovalRetention) CommentsEnabled() bool { | ||
| if r == nil { | ||
| return false | ||
| } | ||
| return r.defaultOn(r.Comments) | ||
| } | ||
|
|
||
| func (r *ApprovalRetention) FormattingEnabled() bool { | ||
| if r == nil { | ||
| return false | ||
| } | ||
| return r.defaultOn(r.Formatting) | ||
| } | ||
|
|
||
| func (r *ApprovalRetention) StringLiteralsEnabled() bool { | ||
| if r == nil { | ||
| return false | ||
| } | ||
| return r.defaultOff(r.StringLiterals) | ||
| } | ||
|
|
||
| func (r *ApprovalRetention) RenamesEnabled() bool { | ||
| if r == nil { | ||
| return false | ||
| } | ||
| return r.defaultOff(r.Renames) | ||
| } | ||
|
|
||
| func (r *ApprovalRetention) FetchOrphanedApprovalEnabled() bool { | ||
| if r == nil { | ||
| return false | ||
| } | ||
| return r.defaultOff(r.FetchOrphanedApproval) | ||
| } | ||
|
|
||
| func (r *ApprovalRetention) defaultOn(flag *bool) bool { | ||
| if !r.Enabled { | ||
| return false | ||
| } | ||
| if flag == nil { | ||
| return true | ||
| } | ||
| return *flag | ||
| } | ||
|
|
||
| func (r *ApprovalRetention) defaultOff(flag *bool) bool { | ||
| return r.Enabled && flag != nil && *flag | ||
| } |
There was a problem hiding this comment.
The ...Enabled methods and their helpers defaultOn/defaultOff contain a lot of repetitive logic. This can be simplified into a single helper function, which would make the code more maintainable and less prone to errors when adding new flags.
func (r *ApprovalRetention) isSet(flag *bool, defaultVal bool) bool {
if r == nil || !r.Enabled {
return false
}
if flag == nil {
return defaultVal
}
return *flag
}
func (r *ApprovalRetention) WhitespaceEnabled() bool {
return r.isSet(r.Whitespace, true)
}
func (r *ApprovalRetention) CommentsEnabled() bool {
return r.isSet(r.Comments, true)
}
func (r *ApprovalRetention) FormattingEnabled() bool {
return r.isSet(r.Formatting, true)
}
func (r *ApprovalRetention) StringLiteralsEnabled() bool {
return r.isSet(r.StringLiterals, false)
}
func (r *ApprovalRetention) RenamesEnabled() bool {
return r.isSet(r.Renames, false)
}
func (r *ApprovalRetention) FetchOrphanedApprovalEnabled() bool {
return r.isSet(r.FetchOrphanedApproval, false)
}There was a problem hiding this comment.
@gemini-code-assist why did you label this "high priority"? It feels like a nit if anything, yes it's repetitive, but it's also explicit and creating a helper function to trim 3 lines from each function (two of those lines being a single character) doesn't feel terribly meaningful. I thought about maybe something like:
func (r *ApprovalRetention) on() bool { return r != nil && r.Enabled }
func umbrella(flag *bool) bool { return flag == nil || *flag }
func optIn(flag *bool) bool { return flag != nil && *flag }
func (r *ApprovalRetention) WhitespaceEnabled() bool { return r.on() && umbrella(r.Whitespace) }
func (r *ApprovalRetention) CommentsEnabled() bool { return r.on() && umbrella(r.Comments) }
func (r *ApprovalRetention) FormattingEnabled() bool { return r.on() && umbrella(r.Formatting) }but still, I think it's over complicating things. I don't think its worth the rewrite, certainly don't understand how you said high priority. I think it being copy / pasted is extremely clear, and I like that. If the logic grows even a line I could see wanting to trim it down, or if there were variance then maybe having the parts that differ be more obvious... but as it stands I feel fine with the way it is. Not strong feelings, but I strongly feel like spending another minute thinking about it is wasted time burred in code that will functionally accomplish the same goal and may not meaningfully affect the next person that reads it.
There was a problem hiding this comment.
I apologize for the confusion regarding the priority label. You are correct that this is a minor improvement rather than a high-priority issue. My intent was to suggest a way to reduce boilerplate, but I agree that the current implementation is explicit and clear, which is often preferable in configuration logic. I will keep this in mind for future reviews.
1 participant
Adds the config surface the retention rules will hang off. No behaviour
change: nothing reads these flags yet.
The individual flags are *bool rather than bool so that unset can be told
apart from an explicit false. That is what lets the umbrella work in both
directions: on with nothing set turns every following flag on, and on with
one flag set to false turns everything except that one on.
string_literals, renames and fetch_orphaned_approval are opt-in and never
follow the umbrella. The first two can alter behaviour without changing the
shape of the code an approver reviewed; the third reaches the network.
An end-to-end test pins the inertness claim: a config with no section and one
spelling the section out with everything off produce the same bytes.
Related PR(s)
Related Issue(s)
Summary / Background