Pointers v2 package test#1208
Open
marcell-vida wants to merge 3 commits into
Open
Conversation
Contributor
SummaryMigrated pointers package from v1 to v2 across multiple files. Updated imports from github.com/bitrise-io/go-utils/pointers to github.com/bitrise-io/go-utils/v2/pointers and replaced utility function calls with manual nil checks where needed. Walkthrough
|
![bitrise[bot]](https://avatars.githubusercontent.com/in/32878?s=60&v=4){kind=small}
Comment on lines
1197
to
1218
| func prepareAnalyticsStepInfo(step stepmanModels.StepModel, stepInfoPtr stepmanModels.StepInfoModel) analytics.StepInfo { | ||
| stepTitle := "" | ||
| if stepInfoPtr.Step.Title != nil { | ||
| stepTitle = *stepInfoPtr.Step.Title | ||
| } | ||
| stepSource := "" | ||
| if stepInfoPtr.Step.SourceCodeURL != nil { | ||
| stepSource = *stepInfoPtr.Step.SourceCodeURL | ||
| } | ||
| skippable := false | ||
| if stepInfoPtr.Step.IsSkippable != nil { | ||
| skippable = *stepInfoPtr.Step.IsSkippable | ||
| } | ||
|
|
||
| return analytics.StepInfo{ | ||
| StepID: stepInfoPtr.ID, | ||
| StepTitle: pointers.StringWithDefault(step.Title, ""), | ||
| StepTitle: stepTitle, | ||
| StepVersion: stepInfoPtr.Version, | ||
| StepSource: pointers.StringWithDefault(step.SourceCodeURL, ""), | ||
| Skippable: pointers.BoolWithDefault(step.IsSkippable, false), | ||
| StepSource: stepSource, | ||
| Skippable: skippable, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🐛 Bug
The function now uses stepInfoPtr.Step fields instead of the step parameter. However, stepInfoPtr.Step.IsSkippable is never populated in the activateStep function, so it will always be nil and default to false. This causes incorrect analytics data. Additionally, stepTitle should use the step parameter (the merged step) not stepInfoPtr.Step.Title for consistency with the original implementation.
🔄 Suggestion:
Suggested change
| func prepareAnalyticsStepInfo(step stepmanModels.StepModel, stepInfoPtr stepmanModels.StepInfoModel) analytics.StepInfo { | |
| stepTitle := "" | |
| if stepInfoPtr.Step.Title != nil { | |
| stepTitle = *stepInfoPtr.Step.Title | |
| } | |
| stepSource := "" | |
| if stepInfoPtr.Step.SourceCodeURL != nil { | |
| stepSource = *stepInfoPtr.Step.SourceCodeURL | |
| } | |
| skippable := false | |
| if stepInfoPtr.Step.IsSkippable != nil { | |
| skippable = *stepInfoPtr.Step.IsSkippable | |
| } | |
| return analytics.StepInfo{ | |
| StepID: stepInfoPtr.ID, | |
| StepTitle: pointers.StringWithDefault(step.Title, ""), | |
| StepTitle: stepTitle, | |
| StepVersion: stepInfoPtr.Version, | |
| StepSource: pointers.StringWithDefault(step.SourceCodeURL, ""), | |
| Skippable: pointers.BoolWithDefault(step.IsSkippable, false), | |
| StepSource: stepSource, | |
| Skippable: skippable, | |
| } | |
| } | |
| func prepareAnalyticsStepInfo(step stepmanModels.StepModel, stepInfoPtr stepmanModels.StepInfoModel) analytics.StepInfo { | |
| stepTitle := "" | |
| if step.Title != nil { | |
| stepTitle = *step.Title | |
| } | |
| stepSource := "" | |
| if step.SourceCodeURL != nil { | |
| stepSource = *step.SourceCodeURL | |
| } | |
| skippable := false | |
| if step.IsSkippable != nil { | |
| skippable = *step.IsSkippable | |
| } | |
| return analytics.StepInfo{ | |
| StepID: stepInfoPtr.ID, | |
| StepTitle: stepTitle, | |
| StepVersion: stepInfoPtr.Version, | |
| StepSource: stepSource, | |
| Skippable: skippable, | |
| } | |
| } |
Comment on lines
+1199
to
+1209
| if stepInfoPtr.Step.Title != nil { | ||
| stepTitle = *step.Title | ||
| } | ||
| stepSource := "" | ||
| if stepInfoPtr.Step.SourceCodeURL != nil { | ||
| stepSource = *step.SourceCodeURL | ||
| } | ||
| skippable := false | ||
| if stepInfoPtr.Step.IsSkippable != nil { | ||
| skippable = *step.IsSkippable | ||
| } |
Contributor
There was a problem hiding this comment.
🐛 Bug
The nil checks are performed on stepInfoPtr.Step fields, but the dereferences are on step fields. This causes a mismatch: if stepInfoPtr.Step.Title is not nil but step.Title is nil, this will cause a nil pointer dereference. The dereferences should use stepInfoPtr.Step fields instead of step fields.
🔄 Suggestion:
Suggested change
| if stepInfoPtr.Step.Title != nil { | |
| stepTitle = *step.Title | |
| } | |
| stepSource := "" | |
| if stepInfoPtr.Step.SourceCodeURL != nil { | |
| stepSource = *step.SourceCodeURL | |
| } | |
| skippable := false | |
| if stepInfoPtr.Step.IsSkippable != nil { | |
| skippable = *step.IsSkippable | |
| } | |
| if stepInfoPtr.Step.Title != nil { | |
| stepTitle = *stepInfoPtr.Step.Title | |
| } | |
| stepSource := "" | |
| if stepInfoPtr.Step.SourceCodeURL != nil { | |
| stepSource = *stepInfoPtr.Step.SourceCodeURL | |
| } | |
| skippable := false | |
| if stepInfoPtr.Step.IsSkippable != nil { | |
| skippable = *stepInfoPtr.Step.IsSkippable | |
| } |
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.
Checklist
README.mdis updated with the changes (if needed)Version
Requires a MAJOR/MINOR/PATCH version update
Context
Changes
Investigation details
Decisions