Skip to content

Conversation

@Paillat-dev
Copy link
Member

No description provided.

Copy link
Member Author

@Paillat-dev Paillat-dev left a comment

Choose a reason for hiding this comment

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

Please run the linter and formatter and fix issues

- Add comprehensive test suite for config functionality
- Fix environment variable loading and type conversion
- Add special handling for hex color values
- Improve code organization and readability
@Paillat-dev Paillat-dev force-pushed the merge-env-with-config branch from 4e9633f to 6195443 Compare February 2, 2025 13:02
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for recursively merging environment variables with a configuration file. The changes update the environment variable parsing logic in the config module and introduce new tests to validate both basic and nested merging scenarios.

  • Updated load_from_env to parse environment variables with the "BOTKIT__" prefix.
  • Introduced merge_dicts for recursive merging of configuration dictionaries.
  • Added tests in tests/config_test.py to verify merging functionality and integration.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/config_test.py Provides unit tests for basic, nested, and integration merging.
src/config/bot_config.py Updates environment variable parsing and adds recursive merging.
Comments suppressed due to low confidence (2)

src/config/bot_config.py:26

  • Environment variable values are stored as strings while tests expect booleans and integers; consider applying type conversion (e.g., via load_json_recursive) to ensure the configuration values have the correct types.
current[part] = value

src/config/bot_config.py:18

  • [nitpick] The return type annotation of load_from_env is overly specific; consider updating it to dict[str, Any] for a more accurate representation of the configuration structure.
def load_from_env() -> dict[str, dict[str, Any]]:

@Paillat-dev Paillat-dev requested a review from Copilot March 27, 2025 14:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds recursive merging of environment variables with a configuration file.

  • Introduces extensive tests in tests/config_test.py to verify dictionary merging and configuration loading.
  • Refactors load_from_env and adds a recursive merge_dicts function in src/config/bot_config.py.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/config_test.py Added tests for basic, nested, and integration config merging.
src/config/bot_config.py Refactored load_from_env and implemented recursive merge_dicts for configuration merging.

@Paillat-dev Paillat-dev requested a review from Copilot March 27, 2025 14:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements recursive merging of environment variables with a YAML configuration file, ensuring nested settings are correctly overridden.

  • Updates the load_from_env function to parse environment variables into nested dictionaries.
  • Implements a recursive merge_dicts function to combine configurations.
  • Adds comprehensive tests to verify both basic and nested merging behaviors.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/config_test.py Adds tests to verify recursive merging and environment variable parsing.
src/config/bot_config.py Modifies environment variable parsing and merging logic to support nested structures.
Comments suppressed due to low confidence (1)

src/config/bot_config.py:22

  • [nitpick] Consider defining a constant for the environment variable prefix (e.g. BOTKIT_PREFIX) instead of hardcoding "BOTKIT__", to improve clarity and maintainability.
values = {k: v for k, v in os.environ.items() if k.startswith("BOTKIT__")}

@Paillat-dev Paillat-dev merged commit 73649b8 into dev Mar 27, 2025
6 checks passed
@Paillat-dev Paillat-dev deleted the merge-env-with-config branch March 27, 2025 14:25
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