feat(campaign): ensure branch names are prefixed with 'turbolift-' correctly #177
Conversation
…rectly - Updated logic in `campaign.go` to apply the 'turbolift-' prefix without duplication. - Added `TestBranchNamePrefixLogic` in `campaign_test.go` to verify prefix logic. - Restored original tests and copyright header in `campaign_test.go`.
sledigabel
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR updates the campaign branch naming logic to ensure all branch names are consistently prefixed with 'turbolift-' without creating duplicates when the prefix already exists.
- Modified the campaign name assignment logic to apply the 'turbolift-' prefix intelligently
- Added comprehensive test coverage for the branch name prefix logic with edge cases
- Minor formatting improvements to import ordering in the test file
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/campaign/campaign.go | Updated Name field assignment to use prefix logic that prevents duplication |
| internal/campaign/campaign_test.go | Added test function and helper to verify prefix behavior with various inputs |
Comments suppressed due to low confidence (1)
internal/campaign/campaign_test.go:273
- The test uses a local helper function
ApplyBranchNamePrefixthat duplicates the logic fromcampaign.goinstead of testing the actual production code. This creates a risk where the test passes but the actual implementation differs. Consider testing the actualOpenCampaignfunction or extracting the prefix logic to a shared function that both production and test code can use.
// ApplyBranchNamePrefix is a helper function to simulate the branch name prefix logic.
internal/campaign/campaign.go
Outdated
| @@ -70,7 +70,7 @@ func OpenCampaign(options *CampaignOptions) (*Campaign, error) { | |||
| } | |||
|
|
|||
| return &Campaign{ | |||
| Name: dirBasename, | |||
| Name: "turbolift-" + strings.TrimPrefix(dirBasename, "turbolift-"), | |||
There was a problem hiding this comment.
[nitpick] The prefix logic is implemented inline using string concatenation and TrimPrefix. This approach works but could be more readable and maintainable if extracted to a dedicated function, especially since there's already a test helper function with similar logic.
| Name: "turbolift-" + strings.TrimPrefix(dirBasename, "turbolift-"), | |
| Name: sanitizeCampaignName(dirBasename), |
Copilot uses AI. Check for mistakes.
…rectly - Updated logic in `campaign.go` to apply the 'turbolift-' prefix without duplication. - Added `TestBranchNamePrefixLogic` in `campaign_test.go` to verify prefix logic. - Restored original tests and copyright header in `campaign_test.go`.
| if name == "" { | ||
| return prefix | ||
| } | ||
| if len(name) >= len(prefix) && name[:len(prefix)] == prefix { |
There was a problem hiding this comment.
This feels a bit clever - why not use strings.HasPrefix?
| @@ -55,6 +54,17 @@ func NewCampaignOptions() *CampaignOptions { | |||
| } | |||
| } | |||
|
|
|||
| func ApplyCampaignNamePrefix(name string) string { | |||
| const prefix = "turbolift-" | |||
| if name == "" { | |||
There was a problem hiding this comment.
If name is empty we're probably in a situation we should not allow ourselves to be in...
campaign.goto apply the 'turbolift-' prefix without duplication.TestBranchNamePrefixLogicincampaign_test.goto verify prefix logic.campaign_test.go.