Pull in the GraphQL API functionality by probablycorey · Pull Request #8 · cli/cli

probablycorey · 2019-10-09T18:46:43Z

This does a few things:

I added an api package and a PullRequests function. I didn't expose a more generic GraphQL function because it seems like an implementation detail. Also, it keeps all the graphql queries and error handling out of our command files.
I attempted to have good error messages. It should give us descriptive errors for HTTP, GitHub API, and GraphQL errors. I hate when API calls fail for unknown reasons 💢
Use the DEBUG env to get log debug info from api calls. The output is not pretty and I have regret that I didn't make it easier to read. But a life without regrets is no life at all.

I didn't add tests. I want to work on that in a follow up PR because this was already getting big.
I did a shitty job with the output of the gh pr list command because the point of this PR is to use GraphQL, not to implement the gh pr list command.

I've left some inline comments below.

probablycorey · 2019-10-09T18:48:35Z

api/client.go

+	success := resp.StatusCode >= 200 && resp.StatusCode < 300
+
+	if success {
+		gr := &graphQLResponse{Data: v}


I'm still getting used to go's variable naming patterns. Is gr a bad name? Is resp better than response? I use v above because I've notice that pattern for interface{} values, but I don't understand why it is v.

i think v is just short for value. i may not agree with go's terseness, but i think gr is a fine variable name in this context. when in rome.

I think golang's "official" stance on guidelines for variable naming is that 1–2-letter (abbreviated) variable names are fine as long as the variable is used close to the place of its assignment.

probablycorey · 2019-10-09T18:53:34Z

api/queries.go

+			PullRequests edges
+		}
+		ViewerCreated   edges
+		ReviewRequested edges


So, I was using the json tag syntax like json:"edges", but when I removed it everything still worked. I can't find any documentation but it seems like go handles this automatically.

by default go tries to auto-guess field names for serializing. if it works, it works.

vilmibm

given that we're in master now i'd like to see us consistently avoiding panic in favor of go-style errors.

api/client.go

vilmibm · 2019-10-09T21:12:28Z

api/client.go

+	success := resp.StatusCode >= 200 && resp.StatusCode < 300
+
+	if success {
+		gr := &graphQLResponse{Data: v}


i think v is just short for value. i may not agree with go's terseness, but i think gr is a fine variable name in this context. when in rome.

vilmibm · 2019-10-09T21:14:00Z

api/queries.go

+			PullRequests edges
+		}
+		ViewerCreated   edges
+		ReviewRequested edges


by default go tries to auto-guess field names for serializing. if it works, it works.

vilmibm · 2019-10-09T21:18:38Z

command/pr.go

-	currentBranch, error := git.Head()
-	if error != nil {
-		panic(error)
+	prPayload, err := api.PullRequests()


this makes perfect sense in the context of the PR and shouldn't change. but for discussion's sake, i've been working on something like this (ignoring errors for brevity's sake):

ghRepo, _ := context.CurrentGitHubRepository() prs, _ := ghRepo.GetPullRequestsByCurrentBranch()

the GitHubRepository struct has methods that would work with api. Does that seem like a good direction?

Love it ❤️

Yess love the verbosity of it ✨

command/pr.go

vilmibm · 2019-10-09T21:21:26Z

command/pr.go

-		panic(error)
+	fmt.Printf("Current Pr\n")
+	if prPayload.CurrentPR != nil {
+		printPr(*prPayload.CurrentPR)


the CurrentPR and similar members are so specific; I wonder if it's worth putting those under some more specific function name like UserPullRequests or something to distinguish between a general list of PRs and the user-oriented lists we've been doing?

i don't know this is probably not urgent

The problem I ran into is how to have specific functions but keep it as one graphql request.

ah yeah, I meant keep the one single query but just name the collective output something more specific than "PullRequests" to indicate that it's a particular bundling / organization of PRs

tierninho · 2019-10-09T21:57:21Z

@probablycorey What is the best way to trigger HTTP, GitHub API, and GraphQL errors? Thank you.

probablycorey · 2019-10-10T02:32:41Z

@tierninho In the future the errors will pre presented better (now they say "panic: blah blah blah")

HTTP errors: turn off your wifi and run the command
GitHub API errors: add some garbage to the front of your token in ~/.config/hub
GraphQL errors: these only happen if you write the query wrong, but if you add some random characters to the query variable in api/queries.go you'll see the errors

mislav

I like how clean is the API client implementation that supports basically only one endpoint!

mislav · 2019-10-10T12:19:26Z

api/client.go

+	success := resp.StatusCode >= 200 && resp.StatusCode < 300
+
+	if success {
+		gr := &graphQLResponse{Data: v}


I think golang's "official" stance on guidelines for variable naming is that 1–2-letter (abbreviated) variable names are fine as long as the variable is used close to the place of its assignment.

mislav · 2019-10-10T12:25:04Z

api/client.go

+func handleResponse(resp *http.Response, body []byte, v interface{}) error {
+	success := resp.StatusCode >= 200 && resp.StatusCode < 300
+
+	if success {


I was surprised that the "success" path is indented here and returns early, while the error path (handleHTTPError) continues in the main function body. The typical golang style is the other way around: error paths are indented and return early, and the most common (expected case) remains at the top level of a function body.

Would such a general guideline make sense for this logic as well?

mislav · 2019-10-10T12:27:33Z

command/pr.go

-		ExecutePr()
+		err := ExecutePr()
+		if err != nil {
+			panic(err) // In the future this should handle the error better, but for now panic seems like a valid reaction


This is the approach we've taken in the prototype branch re: cobra commands that potentially return an error:

RunE: func(cmd *cobra.Command, args []string) error { return ExecutePr() }

I think it's easier to follow that here as well than to panic.

Ahh, I didn't know about the RunE method! Perfect!

mislav · 2019-10-10T12:28:22Z

command/pr.go

-	currentBranch, error := git.Head()
-	if error != nil {
-		panic(error)
+	prPayload, err := api.PullRequests()


Yess love the verbosity of it ✨

mislav · 2019-10-10T12:57:45Z

api/client.go

+	return handleResponse(resp, body, v)
+}
+
+func handleResponse(resp *http.Response, body []byte, v interface{}) error {


I would prefer to use more descriptive argument names than "v" because function signatures can be shown in text editors/IDEs, and them having human-readable argument names helps understand the purpose of those arguments.

I spent too long coming up with a new name and settled on "data". I ended up choosing this because it is what the go documentation uses for json.Unmarshal, which has very similar behavior.

mislav · 2019-10-10T13:00:22Z

api/queries.go

+
+func PullRequests() (PullRequestsPayload, error) {
+	type edges struct {
+		Edges []struct {


TIL:

that one can define nested struct types directly in the parent struct definition

that structs can also be defined within a function call 😲

that structs can also be defined within a function call

I wasn't sure about this pattern. But since the struct is only used in the PullRequests function I figured it keeps everything more contained.

mislav · 2019-10-10T13:07:53Z

api/queries.go

+				pullRequests(headRefName: $headRefName, first: 1) {
+					edges {
+						node {
+							...pr


One thing I observe here is how deeply some of these queries can be indented due to nesting. I'm predicting that we will even go deeper. Perhaps indenting by tab is not the best approach for GraphQL, since tabs are shown so wide by default (8 spaces in GitHub UI), and changes around GraphQL queries might be tricky to review due to online diff formatting. Not a huge issue right now; just something to keep in mind!

My preferred solution would be to use tabs everywhere and make GitHub show tabs as two spaces, but I don't think that's an option. So spaces it is!

vilmibm

errors are consolidated and handling has been moved up, thanks! i'm a fan of mislav's suggestion of putting the panic even higher but i don't think that needs to be addressed here in this PR; i just didn't want a proliferation of new panics

handle errors in port forwarding

probablycorey added 5 commits October 8, 2019 12:04

Add simple graphql function

c5ffa59

Pull out queries

d6eaad1

Add debug output

a24db0f

Rename package to api

52c009e

tweaks

28e4975

probablycorey requested a review from a team October 9, 2019 18:46

probablycorey self-assigned this Oct 9, 2019

Use correct package name

41e0089

probablycorey commented Oct 9, 2019

View reviewed changes

probablycorey added 2 commits October 9, 2019 13:15

Add version

cf60199

spacing

c956bf8

vilmibm suggested changes Oct 9, 2019

View reviewed changes

probablycorey added 2 commits October 9, 2019 15:32

Don't panic

3420220

Use the correct variable name :)

0e260ec

mislav approved these changes Oct 10, 2019

View reviewed changes

vilmibm approved these changes Oct 10, 2019

View reviewed changes

probablycorey added 4 commits October 11, 2019 14:23

A non-successful response returns early

7007a46

Use RunE instead of panicking

04861a1

rename v to data

3fecba6

use spaces

55cbf37

probablycorey merged commit 2b75cd8 into master Oct 11, 2019

mislav deleted the graphql branch October 31, 2019 10:14

mislav unassigned probablycorey Jan 27, 2020

mislav pushed a commit that referenced this pull request Sep 28, 2021

Merge pull request #8 from github/portfwd-errors

fc4c678

handle errors in port forwarding

Conversation

probablycorey commented Oct 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vilmibm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vilmibm Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vilmibm Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tierninho commented Oct 9, 2019

Uh oh!

probablycorey commented Oct 10, 2019

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mislav Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

probablycorey Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vilmibm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

vilmibm Oct 9, 2019 •

edited

Loading

vilmibm Oct 10, 2019 •

edited

Loading

mislav Oct 10, 2019 •

edited

Loading

probablycorey Oct 11, 2019 •

edited

Loading