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.

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

Consolidate and apply tests to the GitHub API object #163

Closed
orta opened this issue Mar 12, 2017 · 2 comments
Closed

Consolidate and apply tests to the GitHub API object #163

orta opened this issue Mar 12, 2017 · 2 comments
Labels
enhancement help wanted You Can Do This This idea is well spec'd and ready for a PR

Comments

@orta
Copy link
Member

orta commented Mar 12, 2017

It's a bit of a mess in there:

some return JSON

  async getUserInfo(): Promise<any> {
    const response: any = await this.get("user")
    return await response.json()
  }

async getReviewerRequests(): Promise<any> {
    const repo = this.ciSource.repoSlug
    const prID = this.ciSource.pullRequestID
    const res = await this.get(`repos/${repo}/pulls/${prID}/requested_reviewers`, {
      accept: "application/vnd.github.black-cat-preview+json"
    })
    if (res.ok) {
      return res.json()
    }
    return []
  }

some return the fetch call

  getPullRequestDiff(): Promise<any> {
    const repo = this.ciSource.repoSlug
    const prID = this.ciSource.pullRequestID
    return this.get(`repos/${repo}/pulls/${prID}`, {
      accept: "application/vnd.github.v3.diff"
    })
  }

some return a string

  async fileContents(path: string, ref?: string): Promise<string> {
    // Use head of PR (current state of PR) if no ref passed
    if (!ref) {
      const pr = await this.getPullRequestInfo()
      const prJSON = await pr.json()

      ref = prJSON.head.ref
    }
    const fileMetadata = await this.getFileContents(path, ref)
    const data = await fileMetadata.json()
    const buffer = new Buffer(data.content, "base64")
    return buffer.toString()
  }

and the names of the functions don't really mean too much. Perhaps the answer is that the the verb is put before, then the return type at the end: turning getReviewerRequests into getReviewerRequestsJSON. It may make sense to consider that two functions:

async getReviewerRequests(): Promise<any> {
    const repo = this.ciSource.repoSlug
    const prID = this.ciSource.pullRequestID
    return await this.get(`repos/${repo}/pulls/${prID}/requested_reviewers`, {
      accept: "application/vnd.github.black-cat-preview+json"
    })
}

async getReviewerRequestsJSON(): Promise<any> {
    const res =  await this.getReviewerRequests()
    if (res.ok) {
      return res.json()
    }
    return []
}
@orta orta added enhancement help wanted You Can Do This This idea is well spec'd and ready for a PR labels Mar 12, 2017
@deecewan
Copy link
Member

my thoughts would be to:

  • only return JSON. The Github API returns JSON for everything.
  • error handling should be done inside the API call (I kinda made a move at this by testing res.ok in the reviewerRequests call

@alex3165
Copy link
Contributor

PR up #173

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted You Can Do This This idea is well spec'd and ready for a PR
Projects
None yet
Development

No branches or pull requests

3 participants