feat: specify multiple configuration files#196
Open
reisbauer03 wants to merge 5 commits intodnhkng:mainfrom
Open
feat: specify multiple configuration files#196reisbauer03 wants to merge 5 commits intodnhkng:mainfrom
reisbauer03 wants to merge 5 commits intodnhkng:mainfrom
Conversation
…iles, with explicit configuration from later files overriding earlier files
Contributor
📝 WalkthroughWalkthroughAdds multi-file configuration support across CLI, core engine, and TUI: CLI accepts repeated Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant Parser
participant Engine
participant TUI
User->>CLI: run command with --config file1 --config file2
CLI->>Parser: parse args via parser_add_config()
Parser->>Parser: collect repeated --config into list
Parser-->>CLI: return args.config (list)
CLI->>Engine: Glados.from_yaml([file1, file2])
Engine->>Engine: normalize to list and load file1 (utf-8 / utf-8-sig fallback)
Engine->>Engine: load file2 and deep-merge (file2 overrides)
Engine-->>CLI: return merged config
CLI->>TUI: GladosUI(config_paths=[file1, file2])
TUI->>TUI: normalize to list[Path], validate existence
TUI->>Engine: GladosConfig.from_yaml(self._config_paths)
Engine-->>TUI: merged configuration
TUI-->>User: render UI with merged config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/glados/cli.py (1)
175-202:⚠️ Potential issue | 🟡 MinorUnused
config_pathparameter insay()function.The
config_pathparameter is declared but never used in the function body. Thesaycommand callssay(args.text, args.config)at line 406, passing the config, but it's ignored.Either remove the parameter or use it to configure the TTS (e.g., loading voice settings from config).
Option 1: Remove unused parameter
-def say(text: str, config_path: str | Path | list[str] | list[Path] = "glados_config.yaml") -> None: +def say(text: str) -> None: """ Converts text to speech using the GLaDOS text-to-speech system and plays the generated audio. Parameters: text (str): The text to be spoken by the GLaDOS voice assistant. - config_path (str | Path | list, optional): Path to the configuration YAML file(s). - Defaults to "glados_config.yaml".And update the call site at line 406:
- say(args.text, args.config) + say(args.text)Option 2: Use the config to load voice settings
def say(text: str, config_path: str | Path | list[str] | list[Path] = "glados_config.yaml") -> None: + config = GladosConfig.from_yaml(config_path) - glados_tts = tts_glados.SpeechSynthesizer() + glados_tts = tts_glados.SpeechSynthesizer(voice=config.voice)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/glados/cli.py` around lines 175 - 202, The say() function currently ignores its config_path parameter; either remove the unused parameter and update the caller that passes args.config (the say(args.text, args.config) call) or use config_path to load TTS settings and pass them into tts_glados.SpeechSynthesizer(); specifically, if keeping it: parse config_path (accepting str/Path/list), load voice/sample_rate/other settings, instantiate tts_glados.SpeechSynthesizer(...) or call its configure(...) with those settings before generate_speech_audio, otherwise remove the config_path parameter from say() signature and update the call site to only pass text.
🧹 Nitpick comments (2)
src/glados/core/engine.py (1)
163-169: Add defensive check for missing nested keys.If a config file doesn't contain the expected nested structure (e.g., missing
Gladoskey), the error message will be a genericKeyError. Consider adding a more descriptive error.💡 Suggested improvement
# Navigate through nested keys for key in key_to_config: - data = data[key] + if not isinstance(data, dict) or key not in data: + raise ValueError(f"Configuration file {path} missing required key '{key}'") + data = data[key]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/glados/core/engine.py` around lines 163 - 169, The loop that navigates nested config keys (the "for key in key_to_config" block) currently assumes every intermediate key exists and will raise a generic KeyError; update it to defensively check for missing keys (e.g., test "if key not in data" or use data.get) while iterating and raise or log a clear, descriptive error that includes the missing key name and the full path (key_to_config) so callers can see which nested key is absent; ensure following code that uses "data.items()" only runs when the nested data is found.src/glados/cli.py (1)
283-297: Custom argparse action has a subtle identity check that may fail on re-parsing.The check
current_list is self.defaultrelies on object identity. This works for the initial parse, but ifparse_argsis called multiple times with the same parser or if the default is recreated, the identity check could fail unexpectedly. Consider using a sentinel value or checking if the list equals the default.💡 More robust approach using a sentinel
+_CONFIG_DEFAULT_SENTINEL = object() + def parser_add_config(parser: argparse.ArgumentParser) -> None: """ Add the '--config' argument to the given parser. Used to reduce repetitions. Args: parser: parse to add the config argument to. """ class ReplaceDefaultArgsAction(argparse.Action): """ A argparse action similar to 'append', but if any argument is specified, the default value is ignored instead of the default argparse behaviour of appending to the default value. """ def __call__(self, p: argparse.ArgumentParser, namespace: argparse.Namespace, values, option_string=None) -> None: current_list = getattr(namespace, self.dest) - if current_list is self.default: + if current_list is _CONFIG_DEFAULT_SENTINEL: # same by identity - ignore default values, new value is the first element of the list current_list = [values] else: # current_list already is custom - append to it current_list = list(current_list) current_list.append(values) setattr(namespace, self.dest, current_list) parser.add_argument( "--config", type=Path, - default=[DEFAULT_CONFIG], + default=_CONFIG_DEFAULT_SENTINEL, action=ReplaceDefaultArgsAction, help=f"Path to configuration file (default: {DEFAULT_CONFIG}). You may specify this option more than once; the configuration settings in the later files override those in earlier ones.", )Then in
main(), check for the sentinel and replace with the actual default:if args.config is _CONFIG_DEFAULT_SENTINEL: args.config = [DEFAULT_CONFIG]However, for typical CLI usage where
parse_argsis called once, the current implementation should work correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/glados/cli.py` around lines 283 - 297, The ReplaceDefaultArgsAction.__call__ uses identity comparison (current_list is self.default) which can fail on re-parsing; replace that logic with a sentinel-based check: set the argument default to a unique sentinel object (e.g., _ARG_DEFAULT_SENTINEL) when calling add_argument, modify ReplaceDefaultArgsAction.__call__ to test for equality to that sentinel (current_list is _ARG_DEFAULT_SENTINEL or current_list == _ARG_DEFAULT_SENTINEL), and in the program entry (e.g., main) convert the sentinel back to the intended default list (e.g., if args.config is _ARG_DEFAULT_SENTINEL: args.config = [DEFAULT_CONFIG]) so the action reliably treats the first provided value as replacing the default across parse runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/glados/tui.py`:
- Around line 1407-1411: The code currently logs an error when some paths in
config_paths don't exist but then calls GladosConfig.from_yaml(config_paths),
which will raise later; change the behavior to fail early with a clear error:
after computing config_paths and detecting not all(p.exists()), raise a
descriptive exception (e.g., FileNotFoundError or ValueError) or call sys.exit
with a message instead of only logger.error so execution stops before calling
GladosConfig.from_yaml; update the block that uses config_paths, logger.error,
and the subsequent GladosConfig.from_yaml call to perform this early-fail.
---
Outside diff comments:
In `@src/glados/cli.py`:
- Around line 175-202: The say() function currently ignores its config_path
parameter; either remove the unused parameter and update the caller that passes
args.config (the say(args.text, args.config) call) or use config_path to load
TTS settings and pass them into tts_glados.SpeechSynthesizer(); specifically, if
keeping it: parse config_path (accepting str/Path/list), load
voice/sample_rate/other settings, instantiate tts_glados.SpeechSynthesizer(...)
or call its configure(...) with those settings before generate_speech_audio,
otherwise remove the config_path parameter from say() signature and update the
call site to only pass text.
---
Nitpick comments:
In `@src/glados/cli.py`:
- Around line 283-297: The ReplaceDefaultArgsAction.__call__ uses identity
comparison (current_list is self.default) which can fail on re-parsing; replace
that logic with a sentinel-based check: set the argument default to a unique
sentinel object (e.g., _ARG_DEFAULT_SENTINEL) when calling add_argument, modify
ReplaceDefaultArgsAction.__call__ to test for equality to that sentinel
(current_list is _ARG_DEFAULT_SENTINEL or current_list ==
_ARG_DEFAULT_SENTINEL), and in the program entry (e.g., main) convert the
sentinel back to the intended default list (e.g., if args.config is
_ARG_DEFAULT_SENTINEL: args.config = [DEFAULT_CONFIG]) so the action reliably
treats the first provided value as replacing the default across parse runs.
In `@src/glados/core/engine.py`:
- Around line 163-169: The loop that navigates nested config keys (the "for key
in key_to_config" block) currently assumes every intermediate key exists and
will raise a generic KeyError; update it to defensively check for missing keys
(e.g., test "if key not in data" or use data.get) while iterating and raise or
log a clear, descriptive error that includes the missing key name and the full
path (key_to_config) so callers can see which nested key is absent; ensure
following code that uses "data.items()" only runs when the nested data is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64667bf7-4036-4ef8-ae05-36fa9702b3cb
📒 Files selected for processing (3)
src/glados/cli.pysrc/glados/core/engine.pysrc/glados/tui.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/glados/tui.py (1)
1441-1446:⚠️ Potential issue | 🟡 MinorUnused
config_pathparameter inrun_app.The
config_pathparameter is defined but never used—the method callscls()without passing any configuration. This appears to be either dead code or an oversight where the parameter should be forwarded to the constructor.🔧 Option 1: Remove unused parameter
`@classmethod` - def run_app(cls, config_path: str | Path = "glados_config.yaml") -> None: + def run_app(cls) -> None: app: GladosUI | None = None # Initialize app to None try: app = cls()🔧 Option 2: Use the parameter (with multi-config support)
`@classmethod` - def run_app(cls, config_path: str | Path = "glados_config.yaml") -> None: + def run_app(cls, config_paths: str | Path | list[str] | list[Path] | None = None) -> None: app: GladosUI | None = None # Initialize app to None try: - app = cls() + app = cls(config_paths=config_paths) app.run()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/glados/tui.py` around lines 1441 - 1446, The run_app method defines a config_path parameter but doesn’t use it; update run_app to forward config_path to the class constructor by changing app = cls() to app = cls(config_path) and ensure the GladosUI constructor (GladosUI.__init__) accepts an optional config_path: str | Path parameter (with a default if needed) and uses/stores it; if you prefer removing the feature instead, delete the config_path parameter from run_app's signature and any related references to keep the API consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/glados/tui.py`:
- Around line 1441-1446: The run_app method defines a config_path parameter but
doesn’t use it; update run_app to forward config_path to the class constructor
by changing app = cls() to app = cls(config_path) and ensure the GladosUI
constructor (GladosUI.__init__) accepts an optional config_path: str | Path
parameter (with a default if needed) and uses/stores it; if you prefer removing
the feature instead, delete the config_path parameter from run_app's signature
and any related references to keep the API consistent.
reisbauer03
force-pushed
the
pr-multiple-config-files
branch
from
0c6abee to
bb79fd3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/glados/core/engine.py (1)
863-877: Consider renaming parameter topathsfor consistency.
GladosConfig.from_yamlusespathsas the parameter name whileGlados.from_yamlusespath. Aligning the naming would improve API consistency.♻️ Suggested rename
`@classmethod` - def from_yaml(cls, path: str | Path | list[str] | list[Path]) -> "Glados": + def from_yaml(cls, paths: str | Path | list[str] | list[Path]) -> "Glados": """ Create a Glados instance from one or more configuration files. Explicitly specified options in later configuration files override options specified in earlier files. Parameters: - path: One or multiple paths to the YAML configuration file(s) containing Glados settings. + paths: One or multiple paths to the YAML configuration file(s) containing Glados settings. Returns: Glados: A new Glados instance configured with settings from the specified YAML file(s). Example: glados = Glados.from_yaml('config/default.yaml') """ - return cls.from_config(GladosConfig.from_yaml(path)) + return cls.from_config(GladosConfig.from_yaml(paths))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/glados/core/engine.py` around lines 863 - 877, Rename the parameter in Glados.from_yaml from path to paths to match GladosConfig.from_yaml and update the docstring and type hints accordingly: change the signature def from_yaml(cls, paths: str | Path | list[str] | list[Path]) -> "Glados", update the docstring parameter name to "paths", and keep the return call as return cls.from_config(GladosConfig.from_yaml(paths)); also search for any internal or external callers expecting the old parameter name and update them to use the new parameter name to maintain API consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/glados/core/engine.py`:
- Around line 155-165: The code currently assumes
yaml.safe_load(path.read_text(...)) returns a mapping, but if the YAML is empty
or `null` it yields None and the subsequent loop over `key_to_config` will raise
a confusing TypeError; update the load-and-decode block around `yaml.safe_load`
(variable `data`, `path`, encodings loop) to catch `UnicodeDecodeError as e` and
re-raise a ValueError using exception chaining (raise ValueError(...) from e),
then after a successful load validate that `data` is not None and is a mapping
(e.g., dict) before iterating over `key_to_config` and if it is None or not a
dict raise a clear ValueError mentioning the `path` and that the YAML is empty
or malformed.
---
Nitpick comments:
In `@src/glados/core/engine.py`:
- Around line 863-877: Rename the parameter in Glados.from_yaml from path to
paths to match GladosConfig.from_yaml and update the docstring and type hints
accordingly: change the signature def from_yaml(cls, paths: str | Path |
list[str] | list[Path]) -> "Glados", update the docstring parameter name to
"paths", and keep the return call as return
cls.from_config(GladosConfig.from_yaml(paths)); also search for any internal or
external callers expecting the old parameter name and update them to use the new
parameter name to maintain API consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
✅ Actions performedReviews resumed. |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/glados/core/engine.py (1)
155-170:⚠️ Potential issue | 🟡 MinorGuard the loaded YAML before traversing
key_to_config.
yaml.safe_load()can still returnNoneor a non-mapping here, sodata = data or dict()just defers the failure into a laterKeyError/TypeErrorwhen one of the layered files is empty, missingGlados, or containsGlados: null. Line 161 also still drops the originalUnicodeDecodeErrorcontext.🛠️ Suggested fix
for encoding in ["utf-8", "utf-8-sig"]: try: data = yaml.safe_load(path.read_text(encoding=encoding)) break - except UnicodeDecodeError: + except UnicodeDecodeError as exc: if encoding == "utf-8-sig": - raise ValueError(f"Could not decode YAML file {path} with any supported encoding") - - data = data or dict() + raise ValueError(f"Could not decode YAML file {path} with any supported encoding") from exc + + if data is None: + raise ValueError(f"YAML file {path} is empty or contains only null") + if not isinstance(data, dict): + raise ValueError(f"YAML file {path} must contain a mapping at the top level") # Navigate through nested keys for key in key_to_config: - data = data[key] + try: + data = data[key] + except KeyError as exc: + raise ValueError(f"Missing config section {'.'.join(key_to_config)} in {path}") from exc + if not isinstance(data, dict): + raise ValueError(f"Config section {'.'.join(key_to_config)} in {path} must be a mapping")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/glados/core/engine.py` around lines 155 - 170, Guard against yaml.safe_load returning None or a non-mapping before iterating key_to_config and preserve UnicodeDecodeError context: after the safe_load call in the loop that reads path.read_text(encoding=...), validate that the returned data is a mapping (e.g., dict) and raise a clear ValueError if it is None or not a dict rather than letting a later loop over key_to_config cause a KeyError/TypeError; also when re-raising the decode failure for the "utf-8-sig" branch, include the original UnicodeDecodeError as context (raise ... from e) so the original exception is preserved; once validated, continue using GladosConfig._dict_deep_merge(config, data) to merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/glados/core/engine.py`:
- Around line 155-170: Guard against yaml.safe_load returning None or a
non-mapping before iterating key_to_config and preserve UnicodeDecodeError
context: after the safe_load call in the loop that reads
path.read_text(encoding=...), validate that the returned data is a mapping
(e.g., dict) and raise a clear ValueError if it is None or not a dict rather
than letting a later loop over key_to_config cause a KeyError/TypeError; also
when re-raising the decode failure for the "utf-8-sig" branch, include the
original UnicodeDecodeError as context (raise ... from e) so the original
exception is preserved; once validated, continue using
GladosConfig._dict_deep_merge(config, data) to merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces the ability to specify multiple configuration files in the CLI arguments, with (explicitly set) options in later configuration files overwriting options set in earlier configuration files.
Since I was already changing the code, I also cleaned up the CLI parsing to be less repetitive.
My use case for this feature: to have a "public" configuration file that can be committed and a secret configuration file with e.g. the LLM API key.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor