-
Notifications
You must be signed in to change notification settings - Fork 433
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
#68 OpenApi Endpoint Properties & Missing ChatEndpoint #69
base: master
Are you sure you want to change the base?
Conversation
by usage of the interfaces of the endpoints
by the ChatEndpoint
fixed auth tests
closes #68 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just gonna comment as i'm a random guy on the internet and I am in no way able to approve/request changes since i'm not the owner of the codebase. But in a mainainability standpoint some things should be refactored i think.
@@ -9,7 +9,7 @@ namespace OpenAI_API | |||
/// <summary> | |||
/// Represents authentication to the OpenAPI API endpoint | |||
/// </summary> | |||
public class APIAuthentication | |||
public class APIAuthentication : IAPIAuthentication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If APIAuthentication
is not meant to be inherited please add the keyword sealed
so it's explicitly defined. This comes with some performance benefits: see this PR from the dotnet runtime: dotnet/runtime#49944
/// <summary> | ||
/// Represents authentication to the OpenAPI API endpoint | ||
/// </summary> | ||
public interface IAPIAuthentication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion POCO's (Plain old C# objects) shouldn't have an Interface. However this isn't a poco as it's having a method within it to validate the key.
Personally i'd extract the Validate logic to a service which can validate stuff (for now the api-key).
If you want this APIAuthentication
to be available through DI, one should look into using the Options Pattern as this is meant to fix that problem.
/// Tests the api key against the OpenAI API, to ensure it is valid. This hits the models endpoint so should not be charged for usage. | ||
/// </summary> | ||
/// <returns><see langword="true"/> if the api key is valid, or <see langword="false"/> if empty or not accepted by the OpenAI API.</returns> | ||
Task<bool> ValidateAPIKey(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of this method does a couple of things. Maybe extract that logic to definitive classes which each do their own thing?
Also this method should end in Async
since it's an asynchronous operation.
I'd change the name of the method to: IsApiKeyValidAsync();
so it reads nicely in if/switch-statements
Here's a quick rundown of what I've done:
I'm confident that these changes will help to enhance the overall quality and maintainability of the codebase. Please let me know if you have any questions or feedback on these changes.