-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Add :branch
placeholder for API calls
#1515
Conversation
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.
Looks great! Two tiny points
pkg/cmd/api/api.go
Outdated
@@ -298,12 +301,22 @@ func fillPlaceholders(value string, opts *ApiOptions) (string, error) { | |||
return value, err | |||
} | |||
|
|||
var branch string | |||
if strings.Contains(value, ":branch") { |
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.
There's a subtle bug here: the placeholderRE
matches only :branch\b
(notice the word boundary), but strings.Contains
matches ":branch" even when it's not on a word boundary.
So I would move the branch lookup logic to the below case ":branch"
block
pkg/cmd/api/api.go
Outdated
IO: f.IOStreams, | ||
HttpClient: f.HttpClient, | ||
BaseRepo: f.BaseRepo, | ||
CurrentBranch: git.CurrentBranch, |
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.
Instead of git.CurrentBranch
, you can pass f.Branch
, since this is already what a cmdutil.Factory
provides and allows the current branch to be mocked easier in tests
Also, improve the handling for the branch placeholder
b707ca6
to
bba2d6d
Compare
@mislav |
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.
✨✨
As discussed in #1288, this PR allows for the use of the
:branch
placeholder on theapi
command.The placeholder is substituted with the current git branch name.
Closes #1288