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

Github API Client #95

Merged
merged 16 commits into from Nov 12, 2018

Conversation

Projects
None yet
4 participants
@f-meloni
Copy link
Member

f-meloni commented Nov 6, 2018

This should solve #17

@f-meloni f-meloni requested a review from orta Nov 6, 2018

@DangerCI

This comment has been minimized.

Copy link

DangerCI commented Nov 6, 2018

Warnings
⚠️ Big PR, try to keep changes smaller if you can

Generated by 🚫 dangerJS

@orta

orta approved these changes Nov 6, 2018

group.leave()
}

group.wait()

This comment has been minimized.

@orta

orta Nov 6, 2018

Member

cool, that worked, do we need the dispatch group? It should wait because danger waits till the end of the process

config = TokenConfiguration(settings.github.accessToken, url: baseURL)
} else {
config = TokenConfiguration(settings.github.accessToken)
}

This comment has been minimized.

@orta

orta Nov 6, 2018

Member

Cool, yep, this is what I expected 👍

@@ -28,7 +29,8 @@ public struct GitHub: Decodable, Equatable {
public let reviews: [GitHubReview]

public let requestedReviewers: GitHubRequestedReviewers


internal(set) public var api: Octokit!

This comment has been minimized.

@orta

orta Nov 6, 2018

Member

internal? ( maybe I don't get how access rights work here)

This comment has been minimized.

@KITSFrancoMeloni

KITSFrancoMeloni Nov 6, 2018

Just the set is internal (the api is set from DangerDSL, then it can not be private(set))

"gists_url": "https://api.github.com/users/ashfurrow/gists{/gist_id}",
"starred_url": "https://api.github.com/users/ashfurrow/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/ashfurrow/subscriptions",

This comment has been minimized.

@orta

orta Nov 6, 2018

Member

^ - this might have got revoked (check your emails) - unless it is one of mine?

This comment has been minimized.

@f-meloni

f-meloni Nov 6, 2018

Member

Is the one on the fixtures, but anyway is just to test it parses it correctly, then shouldn't matter

f-meloni added some commits Nov 6, 2018

@orta

This comment has been minimized.

Copy link
Member

orta commented Nov 12, 2018

Looks good - want to add a changelog entry then you can merge 👍

@orta orta merged commit ccd823f into danger:master Nov 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment