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.

Sign up for GitHub

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

fix(CLI): crash when target/params section is empty #STRINGS-921 #722

Merged
merged 1 commit into from
Nov 27, 2024
Merged

fix(CLI): crash when target/params section is empty #STRINGS-921 #722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clients/cli/cmd/internal/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (cmd *PullCommand) Run(config *phrase.Config) error {
target.Params.Branch = optional.NewString(cmd.Branch)
}

val, ok := localesCache[LocalesCacheKey{target.ProjectID, target.Params.Branch.Value()}]
val, ok := localesCache[LocalesCacheKey{target.ProjectID, target.GetBranch()}]
if !ok || len(val) == 0 {
if cmd.Branch != "" {
continue
Expand Down
9 changes: 8 additions & 1 deletion clients/cli/cmd/internal/pull_target.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ type Targets []*Target
func (targets Targets) GetAllLocalesCacheKeys() []LocalesCacheKey {
localesCacheKeys := []LocalesCacheKey{}
for _, target := range targets {
localesCacheKeys = append(localesCacheKeys, LocalesCacheKey{target.ProjectID, target.Params.Branch.Value()})
localesCacheKeys = append(localesCacheKeys, LocalesCacheKey{target.ProjectID, target.GetBranch()})
}
return localesCacheKeys
}
Expand All @@ -32,6 +32,13 @@ type Target struct {
RemoteLocales []*phrase.Locale
}

func (target *Target) GetBranch() string {
if target.Params != nil {
return target.Params.Branch.Value()
}
return ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering whether this should be an empty string or nil when there is no branch? You could return nil when the return type is *string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that would be quite a bigger change, but I can try

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@theSoenke sorry, I took a look, basically everywhere, branch is a string (not a pointer), so changing it would be a big effort

Things like:

openapi/clients/cli/cmd/internal/pull.go

Line 33 in d9c8271

Branch string

openapi/clients/cli/cmd/internal/push.go

Line 28 in 30ae809

Branch string

openapi/clients/cli/cmd/internal/push_source.go

Line 199 in 30ae809

if branch != "" {

etc etc

Copy link
Collaborator

Choose a reason for hiding this comment

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

no worries! just wanted to check whether it makes sense

}

func (target *Target) CheckPreconditions() error {
if err := paths.Validate(target.File, target.FileFormat, ""); err != nil {
return err
Expand Down
Loading