Merged
Conversation
Also update base precommit hooks to latest version
Also update golang ci lint action to use pinned version rather than latest
create a temporary copy of generator for applying options This follows the principle of least surprise - when you pass options to a function call, you expect them to apply only to that call, not permanently mutate the object
Reviewer's GuideUpdates the Go toolchain version and CI tooling, and refactors the name generator to apply options on a cloned instance instead of mutating the original receiver during name generation. Class diagram for updated nameGenerator.Generate behaviorclassDiagram
class NameGenerator {
<<interface>>
+WithSeparator(separator string) NameGenerator
+Generate(options Options) string
}
class nameGenerator {
-Gender gender
-string prefix
-string suffix
-int lenOfRandomString
-string separator
+WithSeparator(separator string) NameGenerator
+Generate(options Options) string
-generateRandomString(length int) string
-generateRandomNumber(limit int) int
}
NameGenerator <|.. nameGenerator
class Gender {
<<enum>>
Male
Female
NonBinary
}
nameGenerator --> Gender
class Options {
<<function type>>
+apply(generator *nameGenerator)
}
Options ..> nameGenerator
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis pull request updates the project's CI/CD tooling and Go development environment. It bumps the supported Go version to 1.25 in CI, updates linting and pre-commit hook versions, simplifies the dependabot configuration, and refactors the generator to apply options to a copy rather than mutating the original instance. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
nameGenerator.Generate, instead of manually cloning the struct intogen, consider changing the receiver to a value receiver (or adding aclone()helper) so options are applied to a copy more idiomatically and without manual field-by-field copying. - The new
pre-commit-golangconfiguration enables many*-modand*-repohooks (build, test, vet, lint, fmt, imports); you may want to trim this set to only the hooks that add value to keep pre-commit runs fast and avoid redundant checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `nameGenerator.Generate`, instead of manually cloning the struct into `gen`, consider changing the receiver to a value receiver (or adding a `clone()` helper) so options are applied to a copy more idiomatically and without manual field-by-field copying.
- The new `pre-commit-golang` configuration enables many `*-mod` and `*-repo` hooks (build, test, vet, lint, fmt, imports); you may want to trim this set to only the hooks that add value to keep pre-commit runs fast and avoid redundant checks.
## Individual Comments
### Comment 1
<location> `generator.go:193-198` </location>
<code_context>
}
func (namegen *nameGenerator) Generate(options ...Options) string {
+ gen := &nameGenerator{
+ gender: namegen.gender,
+ prefix: namegen.prefix,
+ suffix: namegen.suffix,
+ lenOfRandomString: namegen.lenOfRandomString,
+ separator: namegen.separator,
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Copying individual fields risks missing new fields on nameGenerator and breaking Generate behavior.
Constructing `gen` with an explicit field list means any future fields added to `nameGenerator` (e.g., RNG, config, or internal state) won’t be copied, and `Generate` may behave incorrectly in subtle ways. Instead, shallow-copy the struct and apply options on that copy, for example:
```go
func (namegen *nameGenerator) Generate(options ...Options) string {
gen := *namegen
gptr := &gen
for _, option := range options {
option(gptr)
}
// use gptr (or gen) below
}
```
This preserves non-mutating behavior while keeping all current and future fields intact.
Suggested implementation:
```golang
}
func (namegen *nameGenerator) Generate(options ...Options) string {
gen := *namegen
gptr := &gen
for _, option := range options {
option(gptr)
```
1. Ensure the remainder of the `Generate` function uses `gptr` (or `gen`) consistently for any fields it reads or methods it calls, rather than `namegen`, if the intent is to work on the per-call copy.
2. If `Generate` currently refers to the old `gen` variable in code not shown in the snippet, update those references to `gptr` (or `gen`) according to whether a pointer or value receiver is required.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
generator.go (1)
232-243: Critical: GenerateMultiple still mutates the original generator, creating API inconsistency.While
Generatenow uses a copy-on-write pattern (lines 193-199),GenerateMultiplestill mutates the original generator at lines 233-235. This creates inconsistent behavior:// Case 1: Generate is non-mutating ✓ gen := NewGenerator() name := gen.Generate(WithGender(Male)) // gen is unchanged // Case 2: GenerateMultiple is mutating ✗ gen := NewGenerator() names := gen.GenerateMultiple(10, WithGender(Male)) // gen is now mutated with Male genderThis violates the functional options pattern and contradicts the PR's stated objective to "fix functional option behavior by creating a temporary copy."
🔎 Proposed fix to align GenerateMultiple with Generate's copy-on-write pattern
func (namegen *nameGenerator) GenerateMultiple(count int, options ...Options) []string { + gen := &nameGenerator{ + gender: namegen.gender, + prefix: namegen.prefix, + suffix: namegen.suffix, + lenOfRandomString: namegen.lenOfRandomString, + separator: namegen.separator, + } + for _, option := range options { - option(namegen) + option(gen) } names := make([]string, count) for i := 0; i < count; i++ { - names[i] = namegen.Generate() + names[i] = gen.Generate() } return names }
🧹 Nitpick comments (1)
.pre-commit-config.yaml (1)
18-32: Consider the stability implications of using a release candidate version.The pre-commit hook repository has been migrated from
dnephin/pre-commit-golangtotekwizely/pre-commit-golangand pinned tov1.0.0-rc.4(a release candidate). While the expanded hook set (12 hooks vs 2) significantly improves Go tooling coverage, using an RC version in production may introduce instability or breaking changes before the stable v1.0.0 release.Verify whether a stable version is available:
#!/bin/bash # Description: Check if tekwizely/pre-commit-golang has a stable v1.0.0 or later release echo "Checking available releases for tekwizely/pre-commit-golang..." gh api repos/tekwizely/pre-commit-golang/releases | jq -r '.[].tag_name' | head -10 echo -e "\nChecking if the repository is actively maintained..." gh api repos/tekwizely/pre-commit-golang | jq '{ name: .name, stars: .stargazers_count, last_updated: .updated_at, archived: .archived }'
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/dependabot.yml.github/workflows/ci.yaml.pre-commit-config.yamlgenerator.go
💤 Files with no reviewable changes (1)
- .github/dependabot.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodneyosodo
Repo: 0x6flab/namegenerator PR: 36
File: .github/workflows/ci.yaml:57-57
Timestamp: 2025-06-30T08:33:06.277Z
Learning: Go 1.24 is available in actions/go-versions repository with multiple patch releases (1.24.0, 1.24.1, 1.24.2, 1.24.3, 1.24.4) as of June 2025, so actions/setup-gov5 supports Go 1.24.
📚 Learning: 2025-06-30T08:33:06.277Z
Learnt from: rodneyosodo
Repo: 0x6flab/namegenerator PR: 36
File: .github/workflows/ci.yaml:57-57
Timestamp: 2025-06-30T08:33:06.277Z
Learning: Go 1.24 is available in actions/go-versions repository with multiple patch releases (1.24.0, 1.24.1, 1.24.2, 1.24.3, 1.24.4) as of June 2025, so actions/setup-gov5 supports Go 1.24.
Applied to files:
.github/workflows/ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-across-platforms (windows-latest, 1.25)
- GitHub Check: build-across-platforms (macos-latest, 1.22)
- GitHub Check: testing
🔇 Additional comments (4)
.github/workflows/ci.yaml (3)
58-58: LGTM: Go version update aligns with matrix expansion.The testing job now uses Go 1.25, which is consistent with the matrix addition at line 32. This ensures the CI tests against the latest version.
61-63: LGTM: Pinning golangci-lint version eliminates nondeterminism.Excellent change to pin the golangci-lint version instead of using "latest". This ensures reproducible CI builds and prevents unexpected breakage from new linter versions.
32-32: Go 1.25 is available and properly added to the test matrix.Go 1.25 was released in August 2025, and Go 1.25.5 was released on December 2, 2025, confirming multiple stable patch versions are available with actions/setup-go@v6. The test matrix addition is appropriate.
generator.go (1)
193-228: LGTM: Copy-on-write pattern correctly implemented in Generate.The refactor creates a local copy of the generator state and applies options to the copy rather than mutating the receiver. This ensures that calling
Generatewith options does not modify the original generator instance.However, note that
GenerateMultiple(lines 232-243) still mutates the original generator, creating an API inconsistency (see separate comment).
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Related Tickets & Documents
Added/updated tests?
Added/updated documentation
Notes
Summary by Sourcery
Update Go tooling and CI configuration while making name generation options non-mutating.
Bug Fixes:
Build:
CI:
Chores:
Summary by CodeRabbit
Release Notes
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.