Skip to content
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

Support .NET Standard 2.0 #84

Closed
wants to merge 2 commits into from
Closed

Support .NET Standard 2.0 #84

wants to merge 2 commits into from

Conversation

ricaun
Copy link

@ricaun ricaun commented Jan 25, 2023

Things that were added/changed.

  • Add UnitTest Project for net6.0 and net48
  • Add missing packages for netstandard2.0
  • Change interface methods to extension
  • Change PostAsStreamAsync to async

* Add Extension for Interfaces
* Update HttpClientExtension
* Create Simple Test Project
@kayhantolga
Copy link
Member

thanks for the contribution, but it is quite a big one. Next time if you can create with small changes it would be easier to review.

The first thing I have noticed with these changes we lose the async stream ability. The method waits until the whole response is completed. Please compare in the playground before your changes and after.

@ricaun
Copy link
Author

ricaun commented Jan 26, 2023

I found a little odd that the method PostAsStreamAsync was not async, but the name of the method is so I changed. And the netstandard does not have Send method in the client that's the reason I changed, are you sure the method was working async, I try to create a test for test that but I'm not sure if is working like before.

I only changed that was necessary to make work with netstandard, add the Test just to make sure does not break anything.

@kayhantolga
Copy link
Member

Please check this comment (#73 (comment)). We struggled a bit to make this method work. I will check it to see if it fits with the suggested implementation whenever I have time. currently, in the master branch, it works.

@kayhantolga
Copy link
Member

with .netstandart2 support, we may need to consider creating a new project in the solution. I just created a discussion about this #86. It would be nice to see your thoughts there

@ricaun
Copy link
Author

ricaun commented Jan 27, 2023

I gonna create a new branch and create a unit test project to see how the PostAsStreamAsync works without the change.

@ricaun
Copy link
Author

ricaun commented Jan 27, 2023

I gonna create a new branch and create a unit test project to see how the PostAsStreamAsync works without the change.

I didn't find any difference between the master and the unit_test versions of the CreateCompletionAsStream.
I tested it in the Playground and works in the same way...

@kayhantolga
Copy link
Member

kayhantolga commented Jan 30, 2023

I will prepare a video to show the difference as soon as I find some time.

@kayhantolga
Copy link
Member

https://youtu.be/rGc1X2aD2ms
As I promised I made a video. At the first run(current dev branch), you can see stream data shown as soon as we received it. At the second run(your PR) We are waiting for the completion of the entire stream.

@kayhantolga
Copy link
Member

https://github.com/betalgo/openai/tree/feature/DotNetStandardSupport
I have created a new branch for .dotnet standard support. I think it is better to keep it as a separate project. So this way it doesn't slow us while we update the .net 6 project.
Currently, there are 5 build time errors in the project, which you already solved in your pr. Could you apply your changes there?

@kayhantolga
Copy link
Member

Completed in a different branch and released as part of v6.8.0

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

Successfully merging this pull request may close these issues.

2 participants