🧹 Simplify deeply nested logic in validate_folder_data#797
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 |
![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.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| f"Invalid data from {sanitize_for_log(url)}: rules[{j}].PK must be a string." | ||
| ) | ||
| return False | ||
| return _log_invalid_rules(rules_list, url, "rules") |
There was a problem hiding this comment.
📝 Info: Intentional behavior change: fall-through now correctly returns False
The old inline loops at the two call sites could silently fall through without returning False when _is_valid_rule_list flagged data as invalid but the isinstance()-based fallback loop didn't match any specific error (e.g., a dict subclass passes isinstance(rule, dict) but fails type(r) is not dict). In the old code, this fall-through would continue validation and potentially return True at main.py:1445, accepting data already deemed invalid by the fast-path. The new _log_invalid_rules always returns False (main.py:1358), closing this gap. The docstring at main.py:1338-1345 explicitly documents this design decision. This is a semantic improvement, not a regression.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _log_invalid_rules(rules_list: list[Any], url: str, prefix: str) -> bool: | ||
| """Helper to log specific validation errors for a list of rules.""" | ||
| for j, rule in enumerate(rules_list): | ||
| if not isinstance(rule, dict): | ||
| log.error( | ||
| f"Invalid data from {sanitize_for_log(url)}: {prefix}[{j}] must be an object." | ||
| ) | ||
| return False | ||
| if (pk := rule.get("PK")) is not None and not isinstance(pk, str): | ||
| log.error( | ||
| f"Invalid data from {sanitize_for_log(url)}: {prefix}[{j}].PK must be a string." | ||
| ) | ||
| return False | ||
| return True |
There was a problem hiding this comment.
📝 Info: Existing test coverage validates the refactoring for standard types
The tests in tests/test_folder_validation.py:100-216 and test_main.py:754-797 cover the key scenarios: non-dict rule entries, non-string PK values (int, bool, list), absent PKs, and valid data for both top-level rules and nested rule_groups. All these tests use standard Python types from JSON-like data, so they exercise the happy path of the refactoring correctly. There are no tests with dict subclasses, which is the only path where the type-check mismatch matters.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.72 | 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.
There was a problem hiding this comment.
📝 Info: CONTRIBUTING.md asks for CHANGELOG.md updates but only main.py was changed
CONTRIBUTING.md states: "Update CHANGELOG.md — add an entry under the [Unreleased] section." This PR only modifies main.py and does not update CHANGELOG.md. This is a process guideline for PR submitters rather than a code correctness rule, and it's a minor internal refactor, so not flagging as a bug — but the PR author may want to add a brief entry.
Was this helpful? React with 👍 or 👎 to provide feedback.
🎯 What: Simplified deeply nested validation loops for rules and rule_groups in
validate_folder_databy extracting a new_log_invalid_ruleshelper function.💡 Why: Reduces cyclomatic complexity and improves readability by avoiding 5+ levels of nesting inside loops, making error checking flow linearly and cleaner.
✅ Verification: Verified by checking type safety with
mypyand formatting withruff. Ran the full test suitepytest -vacross the whole codebase, which correctly tests empty and missing rules and type requirements, preserving functionality. Pre-commit hooks were run successfully.✨ Result: A flatter and cleaner
validate_folder_datamethod that delegates rule verification loop logging effectively.