fix(validator): allow frappe in required_apps - #537
mihir-kandoi wants to merge 2 commits into
Conversation
Apps like tally_migrator list "frappe" in hooks.py's required_apps. The check dropped frappe from the declared side only, so it was reported as undeclared even though pyproject.toml declares it. frappe is already guaranteed to be declared, so compare the sets directly. Fixes #485
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # hooks.py's required_apps never lists frappe itself (it's implicit), | ||
| # while pyproject.toml always does - exclude it before comparing. | ||
| missing = sorted(set(self.get_hooks_required_apps(app)) - (set(declared) - {"frappe"})) | ||
| missing = sorted(set(self.get_hooks_required_apps(app)) - set(declared)) |
There was a problem hiding this comment.
frappe in required_apps when pyproject declares it, but docs/app-dependencies.md does not explain that behavior. The repository review rules require relevant documentation to be updated with a behavior change; please add it in this PR.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pilot/core/app/validator/dependency_declarations.py
Line: 33
Comment:
**Dependency docs omit new behavior** The validator now accepts `frappe` in `required_apps` when pyproject declares it, but `docs/app-dependencies.md` does not explain that behavior. The repository review rules require relevant documentation to be updated with a behavior change; please add it in this PR.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def test_dependency_declarations_excludes_frappe_from_hooks_comparison(tmp_path: Path) -> None: | ||
| """pyproject always declares frappe; hooks.py's required_apps never does - | ||
| frappe being present in pyproject alone must not cause any false failure.""" | ||
| """frappe being present in pyproject alone must not cause any false failure.""" |
There was a problem hiding this comment.
test_dependency_declarations_excludes_frappe_from_hooks_comparison, but the comparison no longer excludes frappe. Please rename the test so future readers do not mistake what it verifies.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/pilot/core/test_app_validator.py
Line: 181
Comment:
**Test name misstates behavior** The existing test is still named `test_dependency_declarations_excludes_frappe_from_hooks_comparison`, but the comparison no longer excludes `frappe`. Please rename the test so future readers do not mistake what it verifies.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Apps that list
frappeinhooks.py'srequired_apps(tally_migrator, shop, Letters, sherlock) failed validation with "requires ['frappe'] ... doesn't declare them", even thoughpyproject.tomldeclares frappe. The check removed frappe from the declared side only. frappe is already required inpyproject.toml, so the sets are now compared directly.Fixes #485