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

Updates PR to green #2

Merged
merged 11 commits into from
Mar 14, 2017
Merged

Conversation

orta
Copy link

@orta orta commented Mar 13, 2017

Gets this green - danger#156

I moved the DSL attributes for reviews into the root of the github DSL object, I agree with what you were doing, but it's probably better in the long run that we keep the PR metadata exactly what GitHub gives you ( then it can grow organically as/if GitHub makes changes to the response JSON.

That meant that the test which failed doesn't fail anymore. This is good and bad, so I made danger#164 and danger#163 to try make sure that it fails when we make big changes like that

@orta
Copy link
Author

orta commented Mar 13, 2017

a bunch of the commits are from my work on danger#161 which is now in master

@deecewan
Copy link
Owner

thanks so much for this.

I put reviews/reviewComments into the PR, because that's where they exist inside the the API response. But I'm happy with the change.

@deecewan deecewan merged commit e885ffa into deecewan:feature/github-issue-access Mar 14, 2017
@orta
Copy link
Author

orta commented Mar 15, 2017

ohhh, they do do they, hrm - wonder if it's worth moving them back

@orta
Copy link
Author

orta commented Mar 15, 2017

shoulda read all my comments

@deecewan
Copy link
Owner

yeah, i'm not sure either way. It seems semantic to have them inside of the PR, because they are reviews for the PR. On the other hand, maybe people don't expect it to be in there?

@orta
Copy link
Author

orta commented Mar 15, 2017

I try to keep it as close to the actual response from GH as possible, so if they're there, then we should replicate it IMO

There'll be a pretty solid reference website from all the JSDoc stuff in a couple of months from now, so discoverability shouldn't be an issue IMO

@deecewan
Copy link
Owner

okay, awesome. i agree then, let's emulate the response. I was actually going to ask about a reference site, but that answers the question.

just FYI, there are a lot of overlaps between PR and issues. So the only things that I exposed in issues were the things that weren't in PRs, which is essentially just Labels. I'm not sure how you want to handle that.

@orta
Copy link
Author

orta commented Mar 15, 2017

WRT the issues, that's a great question, maybe it's worth providing the full object also, but providing only the interface for the useful bits like labels? Then we don't need to chop it up, and if something useful is added in the future at least we've got it available form day 1.

@deecewan
Copy link
Owner

Yeah, that's a good call actually. I like that.

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.

2 participants