Vonage/vonage-dotnet-sdk

Enhancement suggestions

lucas404x opened this issue · 4 comments

Hi there, I'm writing to you guys today to suggest some improvements to the SDK based on my experience using it.

  1. The Configurations class reads from appsettings.json, but not from appsettings.Development.json. It's essential this option because in appsettings we usually put production values while in appsettings.Development, well, the development values. As you guys have a sandbox option, I think that supporting both values makes sense.
  2. To create a VonageClient we need to pass a Credentials object to the constructor, which makes sense. But as we already pass the ApiKey and the ApiSecrets to the appSettings, why not add an option where the VonageClient constructs the Credentials object based on the Configuration class? It would be a really cool feature because today I need to read from appSettings to get these values and constructs the Credentials object manually.
  3. The last suggestion is to rename the "appSettings" to "VonageSettings" or something less generic. In projects with dozens of configs, these more specific names help a lot in navigating the project.

I hope that these suggestions are helpful. Thank you for your time and consideration.

Tr00d commented

Hi @lucas404x,

Thanks for all the feedback.
I'll try to provide answers below:

  1. That's a fair point. At the moment, we only from appsettings.json and settings.json (see here). I could do that, but given the file follows appsettings.{Environment}.json, I cannot guarantee we follow the same format because everyone can name their environment according to their preference. Currently, I'm not sure I could use the Environment variable as this is an external library. I'll have to test that.
  2. Indeed, that's a good idea. I could provide a factory method that reads all the data from the configuration file. I'll fit that into my backlog. Regarding the client initialization, I just wanted to let you know that I added something recently where you can register the VonageClient in your service collection. It does allow you to inject the whole client or any sub client. You can have a look here
  3. That's a good point; I will keep that in mind. The tricky part is it's a breaking change for everyone, so I'll have to do that over the next major release (with a migration guide). I still need an ETA as it's not planned anytime soon. The reason is there are a lot of changes I'm willing to make, and I want to avoid making a few major releases in a row.

I hope I was able to answer your questions. I'll keep this issue open for tracking if it's alright for you.

Tr00d commented

Hi @lucas404x,

I'm just getting back to you about your suggestions. I was able to cook something for 1 & 2.

From now on, lazy initialization is available.
I exposed an override to the registration extension method. You can provide your configuration, meaning the settings file for your environment.

// For 'Scoped' lifecycle
builder.Services.AddVonageClientScoped(builder.Configuration);
// Foor 'Transient' lifecycle
builder.Services.AddVonageClientTransient(builder.Configuration);

All clients will be initialized with Credentials built from your configuration values. You no longer have to handle it.
It allows you to have environment-specific appsettings.json.

More details here.

This is not released yet, I assume I'll publish it this week.

Tr00d commented

Hi,

FYI - this feature has been released this week in v6.9.0.

Tr00d commented

Closing this issue, as points 1 & 2 were addressed.
I will adapt the codebase regarding point 3 in the next major release.