Skip to content
This repository has been archived by the owner on May 10, 2022. It is now read-only.

Add token instructions + support for GITHUB_PAT and GITHUB_TOKEN #22

Merged
merged 4 commits into from
Oct 17, 2018

Conversation

isteves
Copy link
Contributor

@isteves isteves commented Oct 1, 2018

In this PR, I made the following changes:

  • added get_tokens() to retrieve tokens in the following order of precedence: 1. GITHUB_GRAPHQL_TOKEN 2. GITHUB_TOKEN 3. GITHUB_PAT
  • changed iterate() to stop and return error message given in responses
  • added a note about tokens in the README

It's probably a good idea to add a test, but I'm not sure what the best way is of doing that.

Relevant issues/code that came up in my discussion with @maelle :
#6
ropensci/ghql#11
https://github.com/r-lib/whoami/blob/master/R/whoami.R#L279
https://github.com/r-lib/gh/blob/bed43d354789d2e4dde75db0327da1d46358f953/R/gh_request.R#L143

use GITHUB_TOKEN or GITHUB_PAT if GITHUB_GRAPHQL_TOKEN is not found
@maelle
Copy link
Contributor

maelle commented Oct 3, 2018

This looks great! And such a great PR description! 👏

To add testing, we'd need to rely on mocking. I wasn't able to find a resource for a how to mock a function depending on its arguments (https://github.com/r-lib/whoami/blob/master/tests/testthat/test-gh-username.R#L25 felt hacky even if it works and was merged 😂).

I wonder whether it'd be overkill to define 3 helpers:

get_github_token <- function(...){
  Sys.getenv("GITHUB_TOKEN", ...)
}
get_github_path <- function(){
  Sys.getenv("GITHUB_PAT")
}

and mock their behavior to see what get_token does. Or at least to test part of the behavior. What do you think?

@maelle
Copy link
Contributor

maelle commented Oct 3, 2018

Two other notes

  • If you don't have time I could work on the tests myself.

  • In any case, please add yourself as "ctb" by installing whoami and then running desc::desc_add_me()! With the dev desc version if you save your ORCID_ID as environment variable, it'll get added as well.

@isteves
Copy link
Contributor Author

isteves commented Oct 4, 2018

What do you think of this test for get_token? I based it off your whoami example + added a function factory (my first on ever!)

context("test-get_token")

test_that("check that correct token is retrieved", {
  mock_sysgetenv <- function(gh_graphql, gh_token, gh_pat) {
    function(x, unset = ""){
      token <- switch(x,
                      "GITHUB_GRAPHQL_TOKEN" = gh_graphql,
                      "GITHUB_TOKEN" = gh_token,
                      "GITHUB_PAT" = gh_pat)
      if(token != "") return(token)
      return(unset)
    }
  }

  with_mock(
    Sys.getenv = mock_sysgetenv("aa", "bb", "cc"),
    expect_equal(get_token(), "aa")
  )

  with_mock(
    Sys.getenv = mock_sysgetenv("", "bb", "cc"),
    expect_equal(get_token(), "bb")
  )

  with_mock(
    Sys.getenv = mock_sysgetenv("", "", "cc"),
    expect_equal(get_token(), "cc")
  )

  with_mock(
    Sys.getenv = mock_sysgetenv("", "", ""),
    expect_error(get_token())
  )
})

@maelle
Copy link
Contributor

maelle commented Oct 4, 2018

wow I might only have written one function factory! It looks great!

@isteves
Copy link
Contributor Author

isteves commented Oct 5, 2018

Ok, I added the test and added myself as a contributor. Couldn't figure out how to use desc so just did it manually 😬

@maelle
Copy link
Contributor

maelle commented Oct 5, 2018

Thank you! Will review next week.

@maelle
Copy link
Contributor

maelle commented Oct 17, 2018

Oops I snoozed the reminder too much! On it now!

@maelle
Copy link
Contributor

maelle commented Oct 17, 2018

Thanks again!

@maelle maelle merged commit 2accb94 into ropensci-archive:master Oct 17, 2018
@isteves
Copy link
Contributor Author

isteves commented Oct 17, 2018

Woot!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants