Conversation
api/queries.go
Outdated
| }{} | ||
| err := client.GraphQL(query, variables, &result) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to determine GH repo ID: %s", err) |
There was a problem hiding this comment.
Since Go 1.13 we can use errors.Wrap for the pattern of annotating errors with more information: https://godoc.org/github.com/pkg/errors#Wrap
There was a problem hiding this comment.
oh hell yeah, thanks
api/queries.go
Outdated
| if err != nil { | ||
| return "", err | ||
| } | ||
| if repoId == "" { |
There was a problem hiding this comment.
Will repoId ever be blank after GitHubRepoId succeeded without error?
There was a problem hiding this comment.
yes; if the target struct to unmarshal into is messed up the graphql call will succeed but repoId will just end up as its zero value. It can only happen if the target struct is wrong (which tests should catch) or if the graphql output changes (which shouldn't happen except when we change graphql versions).
It came up during development/testing and led to a confusing state where I was submitting a bad payload to graphql due to the repoId being blank.
|
|
||
| query := ` | ||
| mutation CreatePullRequest($input: CreatePullRequestInput!) { | ||
| createPullRequest(input: $input) { |
There was a problem hiding this comment.
Good stuff! Following these guides will make it so much more easier for me to implement issue create this week
command/pr_create.go
Outdated
| } | ||
|
|
||
| func init() { | ||
| prCreateCmd.Flags().BoolVarP(&_draftF, "draft", "d", false, |
There was a problem hiding this comment.
If you're wary of defining the variables for these flags on the package-level scope (I can see how the commands package will get noisy like this really soon), you can also skip linking the flag to a variable in the first place:
| prCreateCmd.Flags().BoolVarP(&_draftF, "draft", "d", false, | |
| prCreateCmd.Flags().BoolP("draft", "d", false, "...") |
And then within the command body itself:
isDraft, err := cmd.Flags().GetBool("draft")There was a problem hiding this comment.
whoa TIL, I think I prefer this. Thanks!
|
Thanks for picking this up <3 Given that we're just all in master this week would you like to keep working on this (adding tests, wrapping errors, and cleaning up flag definitions) or should i? |
|
@vilmibm Good question! I'm adding some tweaks right now but after I'm done, feel free to take this and run with it if you have time |
|
sounds good! lemme know when i should take over. |
|
@vilmibm Pushed some tweaks; thank you for your patience |
command/pr_create.go
Outdated
| remote, err := guessRemote(ctx) | ||
| if err != nil { | ||
| return err | ||
| return errors.Wrap(err, "could not determine suitable remote") |
There was a problem hiding this comment.
It looks to me like guessRemote() already adds the "could not determine suitable remote" error text within its implementation? So we can either avoid adding that here, or avoid adding that within guessRemote
|
@mislav do you have thoughts about testing the interactive bits of pr create? we don't need to test that survey works, but it seems like it might be wise to exercise the interactive code path. |
Redefined browse output, and added in more test cases
2 participants
WIP, reopening since I ported to new context/client.
Still working to add tests.