Application v2 - DO NOT MERGE #156
Conversation
There was a problem hiding this comment.
I know there's a couple of compromises in here - although I dislike the || uri.AbsolutePath.StartsWith("/v2/applications")) more than the use of WebClient :)
It looks pretty good apart from a couple of typos. I've also highlighted some concern about both stopping Application from working and deprecating it. Those are really two conflicting actions: deprecation means "this will be removed in the future", removal obviously means it won't work now.
| @@ -61,10 +62,10 @@ public class KeyPair | |||
|
|
|||
| public class VoiceHooks | |||
| { | |||
| public Webhook[] webhooks { get; set; } | |||
There was a problem hiding this comment.
I hate to say it, but 'webhook' is generally one word these days, so the previous version was probably more correct.
There was a problem hiding this comment.
OK, just did a bit more research, and Microsoft seem to consistently use WebHooks, so this is fine as it is. Microsoft! 🤦♂
Nexmo.Api/Application.cs
Outdated
| @@ -98,6 +99,7 @@ public class ApplicationList | |||
| public class ApplicationListResponse : PaginatedResponse<ApplicationList> | |||
| {} | |||
|
|
|||
| [Obsolete("This version of the API has been depricated", true)] | |||
Nexmo.Api/ApplicationV2.cs
Outdated
| /// The private and public keys to be used with the Application. | ||
| /// </summary> | ||
| [JsonProperty("keys")] | ||
| public Keys Keyes { get; set; } |
Nexmo.Api/ApplicationV2.cs
Outdated
| /// The public key used to validate the jwt in a https://en.wikipedia.org/wiki/JSON_Web_Token. | ||
| /// </summary> | ||
| [JsonProperty("private_key")] | ||
| public string Private_key { get; set; } |
There was a problem hiding this comment.
This combination of snake case and camel case seems odd to me - would it be better as PrivateKey?
There was a problem hiding this comment.
Absolutely, I was thinking about the JSON property...
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions
| public ClientMethods.Application Application { get; private set; } | ||
| public ClientMethods.ApplicationV2 ApplicationV2 { get; private set; } |
There was a problem hiding this comment.
It looks like you're no longer initialising Application, but you're leaving the accessor there. I assume this means that if the user tries to access ClientMethods.Application they'll get null?
This seems like a really bad idea. I'd recommend: Either deprecate Application and leave it working, or remove it entirely, so the user's code won't compile if they're accessing it.
There was a problem hiding this comment.
When a class is marked as obsolete, you can't instantiate it anymore, it throws a compiling error. I'll completely remove it from Client. Leaving the class becomes irrelevant also but i'm not sure if completely removing the code is the best decision
Nexmo.Api/ApplicationV2.cs
Outdated
| string url = ApiRequest.GetBaseUriFor(typeof(ApplicationV2), $"/v2/applications/{appId}").ToString(); | ||
| string result; | ||
| var authBytes = Encoding.UTF8.GetBytes(credentials.ApiKey + ":" + credentials.ApiSecret); | ||
| using (WebClient client = new WebClient()) |
| //return response._embedded.Applications; | ||
| } | ||
|
|
||
| public static List<AppResponse> List() |
There was a problem hiding this comment.
Are you missing some docs for this method?
There was a problem hiding this comment.
Nope, meant to be like this :
public static List<AppResponse> List()
{
return List(10, 0);
}
| public ClientMethods.Application Application { get; private set; } | ||
| public ClientMethods.ApplicationV2 ApplicationV2 { get; private set; } |
| using Nexmo.Api.Request; | ||
|
|
||
| namespace Nexmo.Api.ClientMethods | ||
| { | ||
| [Obsolete("This version of the API has been deprecated", true)] |
There was a problem hiding this comment.
Should probably say " ... use ApplicationV2 instead"
| @@ -61,10 +62,10 @@ public class KeyPair | |||
|
|
|||
| public class VoiceHooks | |||
| { | |||
| public Webhook[] webhooks { get; set; } | |||
There was a problem hiding this comment.
OK, just did a bit more research, and Microsoft seem to consistently use WebHooks, so this is fine as it is. Microsoft! 🤦♂
No description provided.