mkht / PSOpenAI

PowerShell module for OpenAI API.
MIT License
35 stars 10 forks source link

To Splats #6

Closed potatoqualitee closed 5 months ago

potatoqualitee commented 5 months ago

Hello! Love your module. I plan to integrate it into a couple of my modules.

I saw that there were a lot of backticks in this project and figured you'd appreciate splats, as they are pretty. Also removed a few other backticks that were not required.

mkht commented 5 months ago

Thanks for the pull request. I had thought about this improvement as well, but the amount of code changes made it seem a bit cumbersome and I hadn't gotten around to it.

However, I feel that uniformly eliminating all backticks is somewhat excessive. I think it would be better to explain some areas, e.g., the codes in the guide, without splatting.

Also, it might be better to leave backticks for pipeline symbols and line breaks after conditional operators to make it clear that the code is continuous on the next line.

On the other hand, the parameters of Invoke-OpenAIAPIRequest are numerous, so it would be very nice to make the code easier to read by splatting.

I would like to review and clarify where I would like to keep the original and where I would accept changes. Please give me some time as there are a lot of changes to the code.

potatoqualitee commented 5 months ago

Thank you for taking the time to review the PR. I have made the requested changes. For the ones that were questionable, I just reverted.

potatoqualitee commented 5 months ago

haha, thank you. i appreciate it 😊

potatoqualitee commented 5 months ago

I will fix the failing tests in a new PR. I'll run them on my side with my own APIs. I needed to set it up anyway.

mkht commented 5 months ago

I was researching that now too. I am certain that this PR change has caused the test to fail, but I still don't know why.

mkht commented 5 months ago

Ah, I got it, it seems we forgot to save the output of Invoke-OpenAIAPIRequest to a variable and just output it.

Other than that, I also found a simple mistake of forgetting to include the Thread ID in the path of the API request.

In any case, I will fix this problem and you don't have to do anything. Thanks for your help. Thanks again.

potatoqualitee commented 5 months ago

Oh, what a relief! Thank you. I was unable to get the tests all green, even when I reverted back to v3.6.0

[+] C:\github\PSOpenAI\Tests\Runs\Start-ThreadRun.tests.ps1 3.21s (3.09s|90ms)
[-] Stop-ThreadRun.Integration tests (online).Create thread 2.83s (2.83s|2ms)
 Expected no exception to be thrown, but an exception "OpenAI API returned an 404 (Not Found) Error: No thread found with id '{0}'." was thrown from C:\github\PSOpenAI\Public\Runs\Stop-ThreadRun.ps1:103 char:21
     +         $Response = Invoke-OpenAIAPIRequest `
     +                     ~~~~~~~~~~~~~~~~~~~~~~~~~.
 at { $script:Result = $script:Run | Stop-ThreadRun -Force -PassThru -TimeoutSec 30 -ea Stop } | Should -Not -Throw, C:\github\PSOpenAI\Tests\Runs\Stop-ThreadRun.tests.ps1:174
 at <ScriptBlock>, C:\github\PSOpenAI\Tests\Runs\Stop-ThreadRun.tests.ps1:174

Any idea how I can get this working locally? I'd like to run them on my repo with my API key so I can ensure I provide solid code.

mkht commented 5 months ago

That's exactly the bug I just found: the API request had to include the Thread ID, but I forgot to include it, so Stop-ThreadRun probably wasn't working correctly to begin with.

I just committed b91a064 for fix this and pushed to main branch. Stop-ThreadRun should work in the current main branch code and in the next release (probably v3.6.1).