UserKeysClient only implements 2 of the 5 calls for this API

[ryangribble]

IUserKeysClient currently only implements 2 of the 5 "public keys" methods on the users/keys GitHub API endpoint

https://developer.github.com/v3/users/keys/

Currently implemented:

List public keys for a user
GET /users/:username/keys

List public keys for the authenticated user
GET /user/keys

Not yet implemented:

Get a single public key
GET /user/keys/:id

Create a public key
POST /user/keys

Delete a public key
DELETE /user/keys/:id

[shiftkey]

I knew this code lived somewhere (ignore the second TODO comment):

// TODO: this should be under Users to align with the API docs
// TODO: this should be named PublicKeys to align with the API docs
/// <summary>
/// Access GitHub's Public Keys API.
/// </summary>
/// <remarks>
/// Refer to the API docmentation for more information: https://developer.github.com/v3/users/keys/
/// </remarks>
public ISshKeysClient SshKey { get; private set; }

So yes, the methods on ISshKeysClient should be added to IUserKeysClient - and IGitHubClient.SshKey should be marked as obsolete for removal as it doesn't align with the API.

[ryangribble]

I knew this code lived somewhere
Ah right, we even use that SshKeyClient at the moment but I was tunnel visioning on the User.Keys side of things and didnt think that they are the same thing!

So yes, the methods on ISshKeysClient should be added to IUserKeysClient - and IGitHubClient.SshKey should be marked as obsolete for removal as it doesn't align with the API.
i'm in the process of doing this now, all the methods are transferred but I have some questions about the obsolescence stuff

I've marked IGitHubClient.SshKey obsolete but that doesn't actually flag several use cases... So should we actually also mark GitHubClient.SshKey obsolete (that gets anybody using a GitHubClient rather than an IGitHubClient). Then we need of course the Observable and IObservable versions as well. Do we go as far to make the actual class SshKeysClient or it's interface ISshKeysClient obsolete so that anybody who directly creates those clients would get the warning?

Given so many places are required to really catch most use cases, I wonder would it actually be better to simply mark the actual Get() GetAll() Create() Delete() methods as obsolete (on the interface and concrete SshKeysClient), that way it doesnt matter whether the consumer uses interfaces or concrete class, the top level client or the low level client... if they invoke any of the methods, they will be advised to move away from them?

So apart from the question on the best way to obsolete something, I've also reviewed the associated code/classes and found that the SshKey and SshKeyInfo classes probably need to be obsoleted as well.

The only place SshKey is used is within the SshKeysClient so once that's obsoleted and eventually removed, it seems the SshKey response object should go too? The PublicKey class has all the same fields and is used in UserKeysClient which aligns with the API terminology of referring to public keys rather than ssh keys.

And how about this ModelExtensions class?
As per the TODO comment it only deals with SshKeys and should have been renamed to something less generic... but Im more so interested in whether I need to create equivalent extension class/methods for PublicKey class (and thus create PublicKeyInfo class too) or can these extensions just be dropped (when SshKey is dropped, after having been obsolete for a while)? These ModelExtensions are only used in the unit/integration tests (within Octokit) though Im not sure if the intention was that consumers of Octokit would use these helper extension methods to do stuff with the contents of keys?

[shiftkey]

👍 to obsoleting the methods rather than the types - that's going to put it in front of impacted users much more clearly

[shiftkey]

And 👍 to obsoleting SshKey - the API documentation refers to "public keys" so let's follow that convention...

[shiftkey]

Let's port those extension methods over to PublicKey and mark the SshKey variants as obsolete.

EDIT: good point on the "only used in tests" thing. Mark these versions as obsolete, move the new variants into the tests (we don't have a shared project for test helpers, which might get in the way here).

[ryangribble]

EDIT: good point on the "only used in tests" thing. Mark these versions as obsolete, move the new variants into the tests (we don't have a shared project for test helpers, which might get in the way here).

Further investigation showed that these extension methods arent even used by any tests other than the unit tests FOR the extension methods themselves (ie ModelExtensionTests.cs)

Perhaps the 3 extension methods and PublicKeyInfo class dont even need to be created at all?

Unless there was some intention that this helper method was exposed to octokit consumers to do stuff with. It's pretty basic anyway, not sure if there would be a real world use case. @haacked ?

[haacked]

Unless there was some intention that this helper method was exposed to octokit consumers to do stuff with. It's pretty basic anyway, not sure if there would be a real world use case. @haacked ?

We use these extension methods in GitHub Desktop. I'd like to keep them, but 👍 to renaming it from ModelExtensions to something more specific. I think the intent was we'd have more extension methods for working with models, but so far, we don't. 😄

[ryangribble]

So given that SshKeyInfo isnt a response class but just a helper class, what would say if we created the PublicKeyInfo class to replace it, and in it's constructor it took a PublicKey item eg var keyInfo = new PublicKeyInfo(key) and/or had static PublicKeyInfo.FromPublicKey(PublicKey key) or do you really want it to be an extension method on PublicKey itself?

[haacked]

or do you really want it to be an extension method on PublicKey itself?

I think the benefit of the extension methods is it made the helper class more discoverable. As you pointed out, the helper class is not a response class so most users of Octokit.net won't know it exists.

However, I don't feel too strongly either way, so whatever you feel is the best design here.