feat(config): add validation for config file paths and improve error … - #1044
feat(config): add validation for config file paths and improve error …#1044wakonig wants to merge 4 commits into
Conversation
bfc4342 to
579709a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new path validation does not ensure the path is a regular file, so a directory named *.yaml/*.yml can slip through and later fail with an unexpected IsADirectoryError instead of a DeviceConfigError.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves bec_lib configuration loading error handling by validating config file paths before checking YAML compatibility, and by suggesting likely .yaml/.yml alternatives when the provided path is missing—avoiding the previously confusing NotImplementedError behavior.
Changes:
- Add explicit config-path validation that raises
DeviceConfigErrorwith clearer messages (and optional “Did you mean …?” suggestions). - Add comprehensive unit tests covering unsupported extensions, missing files, suggestion ordering/limits, and error-tolerant directory scanning.
File summaries
| File | Description |
|---|---|
| bec_lib/bec_lib/config_helper.py | Adds path validation + alternative suggestion logic for missing config files. |
| bec_lib/tests/test_config_helper.py | Adds/extends tests for missing/unsupported config path handling and suggestion behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
579709a to
fee69fd
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are coherent, well-tested, and align with the PR’s stated goal of improving config-loading validation and error reporting without introducing breaking cross-service contract changes.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Description
A typo while loading the config can lead to a confusing "NotImplementedError" as the system first checks against the suffix. This PR changes the config loading by verifying that file exists before checking for compatibility. Moreover, it tries to give a more helpful error message by checking yaml and yml files that the user may have wanted to use instead.
Potential side effects
I decided to not automatically load the found config as this could be quite nasty to debug and lead to unexpected results.
Screenshots / GIFs (if applicable)
Examples: