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

Add support for Github API for querying repository info #63

Merged
merged 7 commits into from
Mar 4, 2020

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Mar 2, 2020

Adds --access command line flag.

There are two values current for the flag: --access=checkout (the original behavior, and remains the default for now) and `--access=github (the new behavior).

In the long term, we will probably want --access=github to be the default, but there are some refinements I think we want before we do that.

In any case, I wanted to get this code up so that it doesn't bitrot further, since it seems like there are parallel efforts that could accelerate bitrot if I don't get it up.


The aforementioned refinements are written out in one of the commits, but here they are again:

  1. To-do: Fallback to local checkout if the github accessor errors out

  2. To-do: Add a warning to stdout if the user runs github access without passing an access token via GITHUB_TOKEN env var (since doing that will hit rate limit much faster).


Fix #40

pnkfelix added 7 commits March 2, 2020 11:48
You turn it on via `--access=github`.

Note A local git checkout is still the default value for `--access`; I
do not want to make github access the default until we make the experience
a bit more user-friendly. Most notable are the following to-do's:

 1. To-do: Fallback to local checkout if the github accessor errors out

 2. To-do: Add a warning to stdout if the user runs github access without
    passing an access token via GITHUB_TOKEN env var (since doing that will
    hit rate limit much faster).
Copy link
Member

@chrissimpkins chrissimpkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great Felix. Need help running through any regression checks to test the changes here? Let me know if I can help.

src/github.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
@spastorino
Copy link
Member

Great work @pnkfelix this looks very good.

Will wait for you to address @chrissimpkins comments and then I'd merge it :).

@pnkfelix
Copy link
Member Author

pnkfelix commented Mar 4, 2020

@spastorino okay I think everything has been resolved now.

(I am separately working on a regression test infrastructure; that side-project has given me sufficient confidence that this does seem to be working. I'll be posting the test system in a separate PR later; its taking me longer to polish than I expected.)

@pnkfelix
Copy link
Member Author

pnkfelix commented Mar 4, 2020

Great work @pnkfelix this looks very good.

Will wait for you to address @chrissimpkins comments and then I'd merge it :).

Actually, I'm going to interpret this as an implicit "r=me" from @spastorino and go ahead and merge this myself.

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

Successfully merging this pull request may close these issues.

Using the Github API instead of a local git repository
4 participants