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

Allows to change the HTTP client in the factory #65

Closed
wants to merge 4 commits into from

Conversation

GromNaN
Copy link
Contributor

@GromNaN GromNaN commented Mar 3, 2023

Symfony integration would be complete if the symfony/http-client could be used. Instead of duplicating the OpenAI::client() factory, I think it is more simple to allow a custom instance of Psr\Http\Client\ClientInterface. The Symfony PSR-18 client would be used.

tests/OpenAI.php Outdated Show resolved Hide resolved
composer.json Outdated
@@ -11,9 +11,10 @@
],
"require": {
"php": "^8.1.0",
"guzzlehttp/guzzle": "^7.5.0"
"psr/http-client-implementation": "1.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to require psr/http-client-implementation instead of a specific implementation. So that it is not necessary to exclude guzzle from projects that want to use an other implementation.

For the Laravel package, guzzle can be explicitly required to keep a smooth installation path.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should also require "php-http/discovery": "^1.15" so that such an implementation is installed automatically (+ the plugin should be disabled as such since we don't need it in for testing this:

    "config": {
        "allow-plugins": {
            "php-http/discovery": false
        }
    },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, and this now misses require guzzlehttp/psr7 explicitly since it was a transitive deps but isn't anymore (expect in dev)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If guzzle is required by the laravel package, can we skip the composer plugin?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then the plugin will do nothing 🤷 if you fear composer asking for enabling the plugin, it's already whitelisted by both laravel and symfony, and it's anyway still better than composer failing to install because of a missing implementation package.
But you might prefer going with ^1.14 so that this doesn't conflict with sentry when it's installed.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
tests/OpenAI.php Outdated Show resolved Hide resolved
Copy link
Contributor Author

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review @nicolas-grekas. I hope maintainers will like this framework-agnostic implementation.

tests/OpenAI.php Outdated Show resolved Hide resolved
Copy link

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on my side thanks.

@gehrisandro
Copy link
Collaborator

Completed in #75

Thanks to @nicolas-grekas @GromNaN for your help and your valuable feedback!

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.

3 participants