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

Streamline Configuration Loading in Conversation Config Module #95

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

Conversation

Mefisto04
Copy link

This PR introduces several minor yet impactful changes to improve the robustness and clarity of the Conversation Configuration Module:

  1. Improved Error Handling:

    • Catches specific exceptions like FileNotFoundError and yaml.YAMLError for clearer error messages.
    • Replaced print statements with logging for better output control.
  2. Streamlined Configuration Loading:

    • Refactored the get_conversation_config_path method to reduce redundancy by using a loop to check for the configuration file in multiple locations.
  3. Simplified Deep Copy Logic:

    • Enhanced clarity in the __init__ method by directly updating self.config_conversation with the loaded configuration, eliminating the need for deep copying.

These changes enhance the overall maintainability and error reporting of the module, providing a clearer understanding of issues during configuration loading.

Code Snippet

# Error Handling
except FileNotFoundError as e:
    logging.error(f"Error locating {config_file}: {str(e)}")
    return None
except yaml.YAMLError as e:
    logging.error(f"YAML Error: {str(e)}")
    return None

# Configuration Loading
for path in [base_path, os.path.dirname(base_path), os.getcwd()]:
    config_path = os.path.join(path, config_file)
    if os.path.exists(config_path):
        return config_path

# Deep Copy Logic
self.config_conversation.update(config_conversation)

@Mefisto04
Copy link
Author

hey @souzatharsis please review this

@souzatharsis
Copy link
Owner

souzatharsis commented Oct 24, 2024 via email

@souzatharsis
Copy link
Owner

automated tests indicate api keys are not found. please check above logs.

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