Skip to content

fix: Exclude .tox from separation of concerns check#438

Open
mantomas wants to merge 1 commit into
ambient-code:mainfrom
mantomas:exclude-tox
Open

fix: Exclude .tox from separation of concerns check#438
mantomas wants to merge 1 commit into
ambient-code:mainfrom
mantomas:exclude-tox

Conversation

@mantomas
Copy link
Copy Markdown

@mantomas mantomas commented May 18, 2026

Description

Similar as .venv, .tox contains only temporary files which are not part of the project - not excluding this directory leads to false negative in Separation of Concerns check.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test coverage improvement

Changes Made

  • added .tox to `src/agentready/assessors/structure.py

Testing

Ran against local project with .tox - separation_of_concerns check passed after the change (original report: File cohesion: 9346/59348 files >500 lines)

  • Unit tests pass (pytest)
  • Integration tests pass
  • Manual testing performed
  • No new warnings or errors

Checklist

  • My code follows the project's code style
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Summary by CodeRabbit

  • Bug Fixes
    • The separation-of-concerns assessor now correctly excludes .tox directories from code quality analysis, preventing false positives in test environment files.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 18, 2026

📝 Walkthrough

Walkthrough

This PR updates the repository assessment tool to skip .tox directories when checking file cohesion and module naming, treating tox environments the same as other excluded directories like .venv, venv, node_modules, and .git.

Changes

Tox directory exclusion

Layer / File(s) Summary
Exclude .tox from assessment scans
src/agentready/assessors/structure.py
Both _check_file_cohesion and _check_module_naming expand their excluded-path filters to skip .tox alongside .venv, venv, node_modules, and .git.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title follows Conventional Commits format (fix: scope) and accurately describes the main change: excluding .tox from the separation of concerns check.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

📉 Test Coverage Report

Branch Coverage
This PR 73.3%
Main 73.7%
Diff ⚠️ -0.4%

Coverage calculated from unit tests only

Copy link
Copy Markdown
Contributor

@jwm4 jwm4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Bill Murdock (with assistance from Claude Code)

Verdict: Approve

The fix is correct and well-scoped. .tox directories (created by the tox testing framework, full of virtualenvs and test artifacts) were being scanned by the SeparationOfConcernsAssessor, causing massive false positives. Adding .tox to the exclusion list in both _check_file_cohesion() and _check_module_naming() is the right fix.

Minor suggestions (non-blocking)

  1. Consider adding .nox too. The nox testing framework is directly analogous to tox and creates a .nox/ directory with the same kind of temporary virtualenvs. Adding it now would prevent the same bug report from a nox user. Could be done here or as a follow-up.

  2. DRY opportunity (pre-existing). The exclusion list is duplicated in both methods. A class-level constant like _SKIP_DIRS = {".venv", "venv", "node_modules", ".git", ".tox"} would prevent them from drifting apart. Not introduced by this PR, just a nice-to-have.

  3. No test added. Not a blocker for a one-line exclusion fix, but a unit test asserting that paths containing .tox are filtered out would guard against regressions.

Nice contribution, thanks!

@jwm4
Copy link
Copy Markdown
Contributor

jwm4 commented May 21, 2026

Let me know if you plan to make any more changes. If not, I will merge as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants