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

stand alone reaction client #1335

Merged
merged 78 commits into from
Jun 15, 2016

Conversation

martinscholz83
Copy link
Contributor

@martinscholz83 martinscholz83 commented May 30, 2016

First preview of an stand alone reaction client. Initially only for commit comments. I created this branch from #1325.

Fixes #1299 #1300 #1301 #1302 #1303

add api

fix GetAllMethod

fix ApiOptions
await Assert.ThrowsAsync<ArgumentException>(() => client.CommitComments.CreateReaction("", "name", 1, new NewReaction(ReactionType.Heart)));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.CommitComments.CreateReaction("owner", null, 1, new NewReaction(ReactionType.Heart)));
await Assert.ThrowsAsync<ArgumentException>(() => client.CommitComments.CreateReaction("owner", "", 1, new NewReaction(ReactionType.Heart)));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You check if NewReaction is null, so probably want to have a test here on that as well

@M-Zuber
Copy link
Contributor

M-Zuber commented May 30, 2016

Some nitpicking on the tests, but otherwise looks clean enough.

I just have some an itch to try and make a generic solution, but I don't think it will end up looking too nice.
(something along the lines of the urls being based of a type param...)

@@ -1,4 +1,5 @@
using System;
using Octokit;
Copy link
Contributor

Choose a reason for hiding this comment

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

inadvertent?

Copy link
Contributor Author

@martinscholz83 martinscholz83 Jun 9, 2016

Choose a reason for hiding this comment

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

I have a strange behavior in visual studio. Vsual Studio tells me that it cannot resolve typ or namespace for types that came from namespace Octokit. But that comes first since yesterday.

unbenannt

So yes, this was inadvertent.

@ryangribble
Copy link
Contributor

2 integration tests failed for me when creating comments on issues, need to use Number rather than Id field of Issue. Ive also highlighted a couple of grammatical/typos in xml comments

@martinscholz83
Copy link
Contributor Author

martinscholz83 commented Jun 9, 2016

Sorry for the grammatical errors

@ryangribble
Copy link
Contributor

No need to apologise! 😀

There was actually a few more places I found, I sent a PR to your branch. Once that's merged I think we are 💎 and ready to merge!

@ryangribble
Copy link
Contributor

I dont know what is up with TravisCI i've retried like 10 times and always gets the same error. But it's not anything to do with this PR specifically since other PRs are getting the same error. So either it's something that's been merged to master (pretty sure I havent merged anything that didnt pass all builds though) or else travis has some kind of consistent issue...

Any ideas @shiftkey ?

@shiftkey
Copy link
Member

@ryangribble not sure, gonna look into it today now I have some bandwidth

@shiftkey
Copy link
Member

@ryangribble I can see a bunch of failures with the common thread being the use of Mono 4.4.0 - which came out a couple of weeks ago.

Looking into whether I can choose a specific version and verify this is a regression...

@shiftkey
Copy link
Member

I'm pretty sure #1376 is The Hero Fix We Need Right Now™

@martinscholz83
Copy link
Contributor Author

martinscholz83 commented Jun 14, 2016

😸

@shiftkey shiftkey modified the milestone: Reactions Support Jun 14, 2016
@ryangribble
Copy link
Contributor

With the 0.20 release 🚢 'ed and the travis issues resolved, it's finally time to merge this puppy!

Good work @maddin2016 congratulations on your first major contribution! It's been a bit of a drawn out one this time around but hopefully it's been a good experience 👍

LGTM

@ryangribble ryangribble merged commit 5dc6e53 into octokit:master Jun 15, 2016
@martinscholz83
Copy link
Contributor Author

🎉 🎉 🎉
200
definitely!! It's so much fun to work on this project.

@martinscholz83 martinscholz83 deleted the stand-alone-reaction-client branch June 28, 2016 08:00
@swillman0416
Copy link

I have known about this site for years and u destroyed me

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.

8 participants