-
Notifications
You must be signed in to change notification settings - Fork 37
XCode: trim the project path. #69
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.
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
Conversation
…it will trim the project path.
characterSetToRemove {"\n", `'`, `"`}
cmd/common.go
Outdated
| return projpth | ||
| } | ||
| characterSetToRemove := []string{"\n", `'`, `"`} | ||
| for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't use infinite loop if possible - include the condition directly in the for (CONDITION) part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example:
for _, character := range []string{"\n", `'`, `"`} {
...
}
Here you can find more info about range: https://gobyexample.com/range
cmd/common.go
Outdated
| projpth = strings.Trim(projpth, `"`) | ||
| projpth = strings.TrimSpace(projpth) | ||
|
|
||
| if !contains(characterSetToRemove, projpth[:1]) && !contains(characterSetToRemove, projpth[len(projpth)-1:]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additionally, we don't want to remove every occurance of ' and " I believe
the project can include ' or " in the file name, as far as I know
we only want to remove the outmost quotes
so, a single strings.Trim(strings.Trim(...)) would be enough instead of the for, IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beside of that, if you ever will need to use something like your contains function, we have that in a package already:
sliceutil.IsStringInSlice(slice, string)
cmd/xcode.go
Outdated
| } | ||
|
|
||
| projpth = trimProjectpath(projpth) | ||
| projectPath = projpth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
projectPath = projpth -> projectPath = trimProjectpath(projpth)
- trimProjectpath method fix - Infinite loop removed. It will trim every ', ", character - contains method removed (Already have one as a util.) common_test.go: The "Multiple" test case has been removed. (Because we don't use multiple trim) xcode.go: clean https://trello.com/c/hgIG0VO9 e90afee commit fix
cmd/xcode.go
Outdated
| } | ||
| projectPath = projpth | ||
|
|
||
| projectPath = trimProjectpath(projpth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strings.Trim function's second parameter is a cutset, which means all leading and trailing Unicode code points contained in cutset will be removed. There is no need for loop over the items in the cutset.
projectPath = strings.Trim(strings.TrimSpace(projpth), "'\"")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it.
Still, I did not remove the trimProjectpath() because with that method it is easier to make unit tests.
- trimProjectpath method fix - strings.Trim function's second parameter is a cutset -> There is no need for loop over the items in the cutset. https://trello.com/c/hgIG0VO9 4894265 commit fix
- cmd/common_test.go: Test_trimProjectpath removed;
XCode: After the user drag and drop the project file to the console, it will trim the project path.
characterSetToRemove {"\n",
',"}