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

References API implementation #238

Merged
merged 18 commits into from
Nov 27, 2013
Merged

Conversation

khellang
Copy link
Contributor

Fixes #130

It's yours if you want it 😄

I'd love some feedback on how you'd like the GetAll overload that takes the subNamespace parameter. I think it might be better to use a enum value here. Do you do any validation on the client anywhere?

I also snuck some namespace additions in here. Tell me if you'd like me to remove this commit.

@shiftkey
Copy link
Member

It's yours if you want it 😄

Real talk: I love how people get to this stuff before I can get around to it. Thanks!

@shiftkey
Copy link
Member

I'd love some feedback on how you'd like the GetAll overload that takes the subNamespace parameter. I think it might be better to use a enum value here. Do you do any validation on the client anywhere?

The sub-namespace is a handy way organise similar references - heads and tags are some of the common conventions, pulls is another one you might have seen - but I don't think there's value in constraining it to a specific enum.

When a sub-namepsace doesn't exist, the server returns 404 - we should check for that and propagate a more helpful exception to the user.

using System.Net;
using System.Threading.Tasks;
using NSubstitute;
using Octokit;

Copy link
Member

Choose a reason for hiding this comment

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

Nice 💄 change - can we drop the whitespace between this and the other using statements?

@shiftkey
Copy link
Member

Looking good so far, most of the feedback is about cosmetic stuff and typos.

Some integration tests would be awesome (we have helpers for creating repositories and disposing them at the end of the test) but I don't consider it a blocker to merging this in.

That "sub-namespace not found" code path I think is the only technical thing I'd like resolved at this stage.

@khellang
Copy link
Contributor Author

Thanks for great feedback! I also think it would be better to just drop the namespaces. They provide no value in the test project, I just added them for consistency.

The whitespace in the using directives is a result of R#'s "optimize using directives", there should be a DotSettings for the sln with a separate one for test projects so it would be easier for contributors to follow the coding/naming style. Want me to file an issue?

I'll fix the stuff mentioned ASAP 😉

BTW, I had trouble building the sln in VS2013. The build seems very brittle, about 3 out of 4 builds breaks with an error complaining about code analysis. I'll add a screenshot to #143

@khellang
Copy link
Contributor Author

One more thing. The GetAll with sub-namespace, what do you think of the name? I see that other clients have GetFor and GetAllFor names. Should we call it GetAllForSubNamespace?

@khellang
Copy link
Contributor Author

@shiftkey Now the only thing missing is the 404 check and error message 😄

@shiftkey
Copy link
Member

Were you going to drop the namespace additions for the tests?

shiftkey added a commit that referenced this pull request Nov 27, 2013
@shiftkey shiftkey merged commit c2be731 into octokit:master Nov 27, 2013
@shiftkey
Copy link
Member

I've opened #242 for us to track that 404 path, and I'll commit a failing integration test so we don't forget about it either.

Thanks for the contribution!

thumbs up

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.

Git DB API: Implement the References API
2 participants