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

Git DB API: Implement the Tags API #131

Closed
haacked opened this issue Oct 31, 2013 · 15 comments
Closed

Git DB API: Implement the Tags API #131

haacked opened this issue Oct 31, 2013 · 15 comments
Assignees

Comments

@haacked
Copy link
Contributor

haacked commented Oct 31, 2013

http://developer.github.com/v3/git/tags/

@ninjanye
Copy link
Contributor

ninjanye commented Nov 1, 2013

I'd like to contribute. Since the 'easy-fixes' have all been gobbled up I thought I'd take a crack at one of the API implementations. Would this one be a good place to start? Seems to be one of the easier ones to implement...

@haacked
Copy link
Contributor Author

haacked commented Nov 1, 2013

If you follow the patterns of the other clients, they should all be "easy fixes" to be honest. 😄

This is definitely a good one to start with. I'm happy to help if you run into problems.

@ninjanye
Copy link
Contributor

ninjanye commented Nov 1, 2013

Ok, thanks. I'll get cracking... do I need to publicly mark this issue as something I am working on or could someone else come along and fix it up quick-sharp?

@haacked
Copy link
Contributor Author

haacked commented Nov 1, 2013

Hopefully people will read the comments and see it's being worked on before they waste their time on it. I guess I could assign it to myself since you're not a member of the Octokit organization.

@ghost ghost assigned haacked Nov 1, 2013
@ninjanye
Copy link
Contributor

ninjanye commented Nov 1, 2013

No, don't worry, just wanted to be sure I wasn't missing anything

@ninjanye
Copy link
Contributor

ninjanye commented Nov 1, 2013

Should IObservableTagsClient be added to the IObservableGitHubClient? I noticed not all observable clients have been added and can't see the reason why?

@haacked
Copy link
Contributor Author

haacked commented Nov 1, 2013

Great question! Let me answer the second part first:

I noticed not all observable clients have been added and can't see the reason why?

Some clients are "sub-clients". For example, when you navigate to the Issues API you'll notice there's an endpoint for issues. But in the right navbar, there are these other APIs such as Assignees and Milestones.

We've tried to mirror this structure. So the IObservableMilestoneClient isn't a direct property of IObservableGitHubClient. Instead, it's a property of the IObservableIssuesClient. And thus you can get to it by going to client.Issues.Milestones.

Should IObservableTagsClient be added to the IObservableGitHubClient?

This case is slightly different from the Issues scenario because there is no top level DB API. It's just a grouping of apis. However, I still think it might be beneficial to group all these DB related clients as properties of some GitData property.

So what I'd suggest is this:

Add the interface IGitDataClient. This interface would not have any methods. It would just have properties. Add IObservableTagsClient as a property of this new interface. Add this new interface as a property of IObservableGitClient

/cc @half-ogre seem legit to you?

@half-ogre
Copy link
Contributor

I like that plan @haacked, although I think IGitDatabaseClient might be a better name, given the general ambiguity of Data (and in the API it's generally referred to as the Git DB API; not sure why we chose Git Data for the menu).

@haacked
Copy link
Contributor Author

haacked commented Nov 1, 2013

👍 to IGitDatabaseClient and have the property be GitDatabase.

@ninjanye
Copy link
Contributor

ninjanye commented Nov 1, 2013

Thanks for the speedy response guys. That sounds good to me, I'll implement that now.

@ninjanye
Copy link
Contributor

ninjanye commented Nov 1, 2013

Just to clarify IGitDatabaseClient should live in the Ocktokit.Reactive project but should not be IObservableGitDatabaseClient

@haacked
Copy link
Contributor Author

haacked commented Nov 1, 2013

Ah, good question. No, all of the client classes in Octokit.Reactive should have Observable in the name.

Octokit

public interface IGitDatabaseClient
{
    ITagsClient Tag { get; }
    IBlobsClient Blob { get; }
    // ...
}
public interface IGitHubClient
{
    // ... new property
    IGitDatabaseClient GitDatabase { get; }

    // ... existing properties
}

Octokit.Reactive

public interface IObservableGitDatabaseClient
{
    IObservableTagsClient Tag { get; }
    IObservableBlobsClient Blob { get; }
    // ...
}
public interface IGitHubClient
{
    // ... new property
    IObservableGitDatabaseClient GitDatabase { get; }

   // ... existing properties
}

@ninjanye
Copy link
Contributor

ninjanye commented Nov 1, 2013

I thought as much but just wanted to clarify. Thanks again.

@ninjanye
Copy link
Contributor

ninjanye commented Nov 1, 2013

Hope the PR is ok (usual first time nerves). Thank you both for your guidance through this. If the Tags implementation is ok I'd be happy to try and complete the remaining DB API tasks.

This was referenced Nov 5, 2013
@shiftkey
Copy link
Member

Closed via #153

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

No branches or pull requests

4 participants