Implement labels CRUD #256
Conversation
|
@paulcbetts if this looks good i'll add some tests as well. |
| /// http://developer.github.com/v3/repos/:owner/:repo/labels/:name | ||
| /// </remarks> | ||
| /// <returns></returns> | ||
| Task<IReadOnlyList<Label>> GetForRepository(string owner, string repo); |
There was a problem hiding this comment.
It'd be great if you could document these signatures like how we document the other clients.
There's probably things in here which aren't relevant to your use case...
/// <summary>
/// Gets a specific <see cref="Authorization"/> for the authenticated user.
/// </summary>
/// <remarks>
/// This method requires authentication.
/// See the <a href="http://developer.github.com/v3/oauth/#get-a-single-authorization">API documentation</a> for more information.
/// </remarks>
/// <param name="id">The ID of the <see cref="Authorization"/> to get</param>
/// <exception cref="AuthorizationException">
/// Thrown when the current user does not have permission to make this request.
/// </exception>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns>The specified <see cref="Authorization"/>.</returns>
|
@andrerod looking good so far, just a few little things about documentation I'd also ❤️ some tests if you've got time! |
|
@shiftkey updated. |
| using Octokit.Tests.Integration; | ||
| using Xunit; | ||
|
|
||
| public class IssuesLabelsClientTests |
There was a problem hiding this comment.
Don't forget to implement IDisposable so we clean up the repository correctly after the test (this is just an xUnit convention)
|
@shiftkey Good point. Updated again. (also added IDisposable for events which was also missing) |
| /// <returns></returns> | ||
| public static Uri IssueLabels(string owner, string repo, int number) | ||
| { | ||
| return "repos/{0}/{1}/issues/{2}/label".FormatUri(owner, repo, number); |
There was a problem hiding this comment.
This should be "repos/{0}/{1}/issues/{2}/labels"
http://developer.github.com/v3/issues/labels/#list-labels-on-an-issue
|
|
||
| await client.GetForIssue("fake", "repo", 42); | ||
|
|
||
| connection.Received().GetAll<EventInfo>(Arg.Is<Uri>(u => u.ToString() == "repos/fake/repo/issues/42/labels")); |
There was a problem hiding this comment.
When you're testing NSubstitute mocks, you'll need to ensure the right generic parameters are specified.
So these tests should look for <Label> - that's one of the reasons why these tests fail
|
@andrerod can you also run |
| public async Task CanListLabelsForAnIssue() | ||
| { | ||
| var newIssue = new NewIssue("A test issue") { Body = "A new unassigned issue" }; | ||
| var newLabel = new NewLabel("test label", "#FFFFFF"); |
There was a problem hiding this comment.
The color value must not contain the leading #
Can we do some validation in here to ensure users supply this in the correct format?
|
Done |
| get { return _color; } | ||
| set | ||
| { | ||
| if (!string.IsNullOrEmpty(value) |
There was a problem hiding this comment.
I think this logic needs to be flipped, because valid inputs end up throwing the exception
|
@shiftkey : updated and added the same validation logic on the update class. |
| { | ||
| if (!string.IsNullOrEmpty(value) | ||
| && value.Length == 6 | ||
| && Regex.IsMatch(value, @"\A\b[0-9a-fA-F]+\b\Z")) |
There was a problem hiding this comment.
Not that I'm a Regex guru, but could you just condense this down to:
\A\b[0-9a-fA-F]{6}\b\Z
and make the if statement more terse?
There was a problem hiding this comment.
cc @haacked for his regex opinions:
http://developer.github.com/v3/issues/labels/#create-a-label
{
"name": "API",
"color": "FFFFFF"
}
color: Required. A 6 character hex code, without the leading #, identifying the color.
| set | ||
| { | ||
| if (string.IsNullOrEmpty(value) | ||
| || value.Length != 6 |
There was a problem hiding this comment.
Could the length check be added to the Regex check?
And I think the null check will be caught by the regex too so that's probably redundant here...
| get { return _color; } | ||
| set | ||
| { | ||
| if (!Regex.IsMatch(value, @"\A\b[0-9a-fA-F]{6}\b\Z")) |
There was a problem hiding this comment.
You were right, this throws a different exception to the one we want.
We've been using ctor checks (like this one) to catch most of the bad input like that. Would that be worth plugging into these classes?
There was a problem hiding this comment.
Meh. Argument null is probably the right exception anyways. So I think you were actually right although both would work.
As for the ctor check, i'm using the property setter to set the value in the constructor, so this check will happen there as well so IMO I think we're good in that regard. Unless you have a different opinion :)
There was a problem hiding this comment.
As for the ctor check, i'm using the property setter to set the value in the constructor,
It'd be good to enforce such checks for the ctor anyway ("let's send a null label name to the API!" for example)
There was a problem hiding this comment.
I added the null checks in the ctor. Is that what you had in mind ?
There was a problem hiding this comment.
Exactly (well, ArgumentNotNullOrEmptyString specifically but I wasn't clear on that)
|
Right, I'm all happy with this now. Thank for the contribution! |
|
You're welcome! Thanks for the effort and for taking it. |
|
p.s.- don't forget to close the associated issue. |
Implements:
#254