-
Notifications
You must be signed in to change notification settings - Fork 46
Refactor helpers into separate modules #442
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
Conversation
|
@copilot I've changed the target branch this will be merged into. It now has merge conflicts that I need you to resolve |
There was a problem hiding this 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 successfully refactors the large helpers.py file into a well-organized helpers/ package with modules grouped by functionality. The refactoring maintains full backward compatibility while improving code organization and fixing several bugs.
Key Changes
- Split 785-line
helpers.pyinto 5 focused modules:dict_utils.py,file_utils.py,inventory.py,results.py, andmisc.py - Fixed a
NameErrorbug infind_origin()by properly initializing the_framevariable - Removed buggy post-processing code in
container.pythat would have caused anAttributeError - Corrected spelling: "Uknown" → "Unknown"
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| broker/helpers/init.py | Provides backward compatibility by re-exporting all helpers from submodules with complete __all__ list |
| broker/helpers/dict_utils.py | Dictionary manipulation utilities (clean_dict, merge_dicts, flatten_dict, dict_from_paths) |
| broker/helpers/file_utils.py | File handling utilities (load_file, FileLock, yaml operations, temporary files) |
| broker/helpers/inventory.py | Inventory management (load_inventory, update_inventory, inventory_fields_to_dict) with proper circular import handling |
| broker/helpers/results.py | Result and testing classes (Result, MockStub, eval_filter, FilterTest) |
| broker/helpers/misc.py | Miscellaneous helpers (Emitter, find_origin with NameError fix, timeout translation, table formatting) |
| broker/providers/container.py | Removed buggy code that would attempt .split() on a tuple |
| pyproject.toml | Version bump to 0.7.3 |
| uv.lock | Version bump to 0.7.3 |
| .pre-commit-config.yaml | Updated ruff to v0.14.7 |
| .gitignore | Added .venv to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot you've received some review feedback. Please consider the comments as well as their real implication given the context of the comment and decide whether you should accept that change or involve a little more involved code change because of it. |
Resolved the merge conflicts in commit 339713f. Updated the helpers module to use Python's standard logging instead of logzero, matching the logging refactor in the 0.8 branch. Also added |
Addressed the review feedback in commit 7d6c284:
|
I'm unable to perform a proper rebase as I don't have access to force push operations. The Could you either:
Alternatively, if you'd prefer me to work within my constraints, I can try to identify and remove the duplicate changes, though this won't clean up the commit history. |
updates: - [github.com/astral-sh/ruff-pre-commit: v0.14.6 → v0.14.7](astral-sh/ruff-pre-commit@v0.14.6...v0.14.7)
This previously combined old and new logic to result not only in buggy behavior, but also a loss of valuable information. We'll simplify by just not modifying the origin information.
7d6c284 to
818ade0
Compare
- Split the monolithic helpers.py file into separate modules by functionality: - dict_utils.py: Dictionary manipulation utilities - file_utils.py: File handling utilities - inventory.py: Inventory management utilities - results.py: Result and MockStub classes - misc.py: CLI and miscellaneous utilities - Created broker/helpers/__init__.py to re-export all functions for backward compatibility. - Updated logging configuration to use standard python logging. - Addressed code review feedback regarding imports and type checking.
818ade0 to
36e4e64
Compare
__eq__method toMockStubclass__getitem__logicOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.