🧹 Refactor: Extract helper functions from sync_profile#792
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
PR SummaryLow Risk Overview The extracted helpers preserve the existing behavior for parallel URL validation/fetch, dry-run plan generation (including legacy vs multi-action rule formats), and the pre-sync phase that verifies access, optionally deletes matching folders, overlaps rule fetching with deletions, and waits for deletion propagation when needed. Reviewed by Cursor Bugbot for commit 50d6908. Configure here. |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| main.py | 1.66 → 1.80 | Brain Method, Complex Method, Bumpy Road Ahead, Overall Code Complexity, Deep, Nested Complexity |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| existing_rules = set() | ||
| if existing_folders_and_rules[0] is None: | ||
| return False | ||
| existing_folders, existing_rules = existing_folders_and_rules |
There was a problem hiding this comment.
📝 Info: Unused existing_folders variable after unpacking
At main.py:2630, existing_folders is unpacked from the tuple but never used afterward in sync_profile. In the old code, existing_folders was also not used after the deletion/scanning phase — all mutations happened inline before creating SyncContext. This is harmless but the variable could be replaced with _ to signal intent: _, existing_rules = existing_folders_and_rules.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return fetch_folder_data(url) | ||
| return None | ||
|
|
||
| with concurrent.futures.ThreadPoolExecutor() as executor: |
There was a problem hiding this comment.
📝 Info: ThreadPoolExecutor default max_workers in _fetch_all_folder_data
At main.py:2422, the ThreadPoolExecutor() is created without an explicit max_workers argument, defaulting to min(32, os.cpu_count() + 4). This matches the original inline code. For a large number of folder URLs, this could create many concurrent DNS lookups and HTTP fetches. This is a pre-existing design choice, not introduced by the refactoring, but worth noting if the number of folder URLs could be very large.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Start fetching rules from kept folders in background (parallel to deletions) | ||
| existing_rules_future = shared_executor.submit( | ||
| get_all_existing_rules, client, profile_id, folders_to_scan | ||
| ) | ||
|
|
||
| if not no_delete: | ||
| deletion_occurred = False | ||
| if folders_to_delete: | ||
| # Parallel delete to speed up the "clean slate" phase | ||
| # Use shared_executor (3 workers) | ||
| future_to_name = { | ||
| shared_executor.submit( | ||
| delete_folder, client, profile_id, name, folder_id | ||
| ): name | ||
| for name, folder_id in folders_to_delete | ||
| } |
There was a problem hiding this comment.
📝 Info: Potential executor saturation in _prepare_folders_and_rules
At main.py:2523, get_all_existing_rules is submitted to shared_executor (which has DELETE_WORKERS=3 max workers). Then at lines 2532-2536, delete_folder tasks are also submitted to the same executor. If there are ≥3 folders to delete, the background rules-fetch task occupies 1 of 3 slots, leaving only 2 for deletions. This is identical to the pre-refactoring behavior and get_all_existing_rules uses its own internal ThreadPoolExecutor(max_workers=5) for parallel work, so there's no deadlock risk — just slightly reduced delete parallelism.
Was this helpful? React with 👍 or 👎 to provide feedback.
🎯 What: Extracted three helper functions (
_fetch_all_folder_data,_build_plan_entry, and_prepare_folders_and_rules) from the complexsync_profilefunction.💡 Why:
sync_profilewas too long and had high cyclomatic complexity, making it difficult to read and maintain. Breaking it down into logical sub-steps improves code health and modularity.✅ Verification: Ran
uv run pytest(369 passing tests),uv run mypy .(no type errors), anduv tool run ruff check .anduv tool run ruff format ..✨ Result: The
sync_profilefunction is significantly shorter and more readable without altering any functionality.===== ELIR =====
PURPOSE: Refactor
sync_profileinto smaller functions.SECURITY: No security implications as no functionality was altered.
FAILS IF: Extracted logic behaves differently, which was tested extensively via test suite.
VERIFY: Review functionality mapping between extracted functions and the previous unified code.
MAINTAIN: The extraction was mechanical and semantic, keep an eye out for more extraction opportunities in
sync_profile.