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

Bug fixes client.py #114

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Bug fixes client.py #114

wants to merge 1 commit into from

Conversation

aizafaraz
Copy link

@aizafaraz aizafaraz commented Nov 30, 2024

Mutable Default Argument:
Issue: The original code used provider_configs: dict = {} as a default argument. This could lead to unintended side effects due to Python's handling of mutable defaults.
Fix: Changed provider_configs to None and initialized it with {} inside the method.

Repetitive Validation:
Issue: Both Client and Completions contained redundant logic for provider validation.
Fix: Centralized validation logic into _validate_provider_key and reused it.

Unclear Error Handling:
Improved error messages for easier debugging.

Lazy Initialization:
Ensured lazy initialization for properties (chat and completions).

Mutable Default Argument:

Issue: The original code used provider_configs: dict = {} as a default argument. This could lead to unintended side effects due to Python's handling of mutable defaults.
Fix: Changed provider_configs to None and initialized it with {} inside the method.
Repetitive Validation:

Issue: Both Client and Completions contained redundant logic for provider validation.
Fix: Centralized validation logic into _validate_provider_key and reused it.
Unclear Error Handling:

Improved error messages for easier debugging.
Lazy Initialization:

Ensured lazy initialization for properties (chat and completions).
@SermetPekin
Copy link

This is a duplicate PR. There is already a PR with the same idea which waits to be approved opened earlier.

https://github.com/andrewyng/aisuite/pull/84

#84

@aizafaraz
Copy link
Author

aizafaraz commented Nov 30, 2024 via email

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