Skip to content

Endpoint Properties in IOpenAIAPI interface #68

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

Closed
pandapknaepel opened this issue Mar 9, 2023 · 3 comments
Closed

Endpoint Properties in IOpenAIAPI interface #68

pandapknaepel opened this issue Mar 9, 2023 · 3 comments

Comments

@pandapknaepel
Copy link
Contributor

I have made an error in the IOpenAIAPI interface (and the OpenAIAPI class). The endpoint properties should be defined as interfaces instead of classes so that they are also suitable for Moq tests. Additionally, the IChatEndpoint interface should be added as a property.

Wrong:

CompletionEndpoint Completions { get; }

Correct:

ICompletionEndpoint Completions { get; }
pandapknaepel added a commit to pandapknaepel/OpenAI-API-dotnet that referenced this issue Mar 9, 2023
pandapknaepel added a commit to pandapknaepel/OpenAI-API-dotnet that referenced this issue Mar 9, 2023
by usage of the interfaces of the endpoints
pandapknaepel added a commit to pandapknaepel/OpenAI-API-dotnet that referenced this issue Mar 9, 2023
by the ChatEndpoint
pandapknaepel added a commit to pandapknaepel/OpenAI-API-dotnet that referenced this issue Mar 9, 2023
fixed auth tests
@Baklap4
Copy link

Baklap4 commented Mar 10, 2023

Hmm personally i think we're trying to do way more in this single class than just one thing and therefore separation might be better. Even worse we're also replicating a lot of stuff amongst other endpoint classes. The things we're now trying to do per endpoint:

  1. Define some models
    • Request
    • Result
  2. Logic to call CompletionsEndpoint
  3. Logic to stream Results
  4. Helper methods

Then for the logic to call/stream a certain api the stuff is replicated. See: Embedding, Files, Images, Chat.

How about the following:

  1. Have a class & interface responsible for managing HTTP calls
  2. Have a class & interface responsible for managing Streaming calls
  3. Have repositories (also class + interface) for the calls we wanna make for example: CompletionRepository
    • Defines the api to use in a constant
    • This class is responsible for calling the above classes through their interface
    • Validates in/output
    • Returns the data
  4. Generic exceptionhandling middleware
  5. Usage of Extension methods for certain helper functionality on the models
  6. Nice bonus: A validation class to validate certain stuff

With this approach every class is responsible for 1 task. This also makes the classes shorter and easier to comprehend and at last makes the testing way easier as you don't have to mock/stub as much since the logic is spread around more classes which have their own tests to back things up.

@pandapknaepel
Copy link
Contributor Author

pandapknaepel commented Mar 10, 2023

These are good ideas that apply to the project in general. A separate ticket could certainly be created for this. My interest in this ticket is only to be able to test services with Moq that use this NuGet package. I am not the developer of the package.
That's why I tried not to include any breaking changes, etc. in my PR.

And otherwise, I'll build a wrapper around the NuGet package and retire here.

@Baklap4
Copy link

Baklap4 commented Mar 10, 2023

Agreed to hit those refactors off in another ticket

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

2 participants