Conversation
Update mkdocs, fix rules issues, update unit tests
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prepares the project for a beta release by refactoring the core rule/registry architecture, adding a suite of DSL and linter rules, and updating the CLI/docs/tests to match the new configuration surface (notably isolate_services and expanded rule support).
Changes:
- Introduces
BaseRegistry+ refactorsDSLRegistry/LinterRegistryto centralize validation and auto-load rules fromProjectSpec. - Adds many new DSL rules (filesystem/import/naming) plus new linter rules (
service_isolation,no_private_imports,no_wildcard_imports) and shared “checks” utilities. - Updates CLI/config/spec loading, documentation, and tests for the beta release and the new rule/config model.
Reviewed changes
Copilot reviewed 79 out of 81 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_spec_loader.py | Removes strict-option tests in favor of updated config defaults. |
| tests/unit/test_service_isolation_rule.py | Adds unit tests for service isolation linter behavior. |
| tests/unit/test_registries.py | Updates registry tests to the new LinterRegistry API/behavior. |
| tests/unit/test_must_contain_folders_rule.py | Updates import path for MustContainFoldersRule after DSL move. |
| tests/unit/test_dependencies_rule.py | Updates expectations to reflect stricter dependency enforcement. |
| tests/unit/test_config.py | Updates tests to new config module path and new defaults (isolate_services). |
| tests/unit/test_allowed_service_dependencies_rule.py | Adds unit tests for cross-service allowlist enforcement. |
| tests/unit/dsl/test_no_wildcard_imports_rule.py | Adds DSL unit tests for wildcard-import rule. |
| tests/unit/dsl/test_no_test_files_in_rule.py | Adds DSL unit tests for disallowing test files in production folders. |
| tests/unit/dsl/test_no_relative_imports_rule.py | Adds DSL unit tests for relative-import restrictions. |
| tests/unit/dsl/test_no_private_imports_rule.py | Adds DSL unit tests for private-import restrictions. |
| tests/unit/dsl/test_no_files_in_folder_rule.py | Adds DSL unit tests for “no files directly in folder”. |
| tests/unit/dsl/test_no_circular_imports_rule.py | Adds DSL unit tests for cycle detection. |
| tests/unit/dsl/test_must_contain_files_rule.py | Adds DSL unit tests for required files. |
| tests/unit/dsl/test_max_depth_rule.py | Adds DSL unit tests for max directory depth. |
| tests/unit/dsl/test_layer_must_not_import_rule.py | Adds DSL unit tests for hard layer import bans. |
| tests/unit/dsl/test_forbidden_external_libs_rule.py | Adds DSL unit tests for forbidding specific third-party libs. |
| tests/unit/dsl/test_files_must_match_pattern_rule.py | Adds DSL unit tests for filename glob enforcement. |
| tests/unit/dsl/test_files_must_be_snake_case_rule.py | Adds DSL unit tests for snake_case filenames. |
| tests/unit/dsl/test_classes_must_match_pattern_rule.py | Adds DSL unit tests for class-name regex enforcement. |
| tests/unit/dsl/test_allowed_external_libs_rule.py | Adds DSL unit tests for allowlisting third-party libs. |
| tests/unit/dsl/init.py | Adds DSL test package marker. |
| tests/cli/test_cli_init_project.py | Updates init-project expectations to new defaults (isolate_services). |
| tests/cli/test_cli_check.py | Updates CLI check tests for new config handling. |
| src/pyarchrules/pyarchrules.py | Refactors public API: registry construction from spec; split DSL vs linter execution. |
| src/pyarchrules/model/spec/service_spec.py | Expands service spec with new flags (shared, import lint toggles) and docs. |
| src/pyarchrules/model/spec/project_spec.py | Replaces removed strict/fail-on-warning with isolate_services. |
| src/pyarchrules/model/rules/rule_violation.py | Adds docs and clarifies RuleViolation structure. |
| src/pyarchrules/model/rules/rule_eval_result.py | Adds docs for aggregated evaluation results and counts. |
| src/pyarchrules/core/spec_loader.py | Adds key validation, root path handling, and new service/project keys. |
| src/pyarchrules/core/rules/rule_set.py | Greatly expands fluent DSL surface and rule imports. |
| src/pyarchrules/core/rules/rule.py | Improves abstract base rule documentation and typing. |
| src/pyarchrules/core/rules/linter/service_isolation_rule.py | Adds linter rule enforcing cross-service isolation unless shared=true. |
| src/pyarchrules/core/rules/linter/no_wildcard_linter_rule.py | Adds TOML-driven linter rule for wildcard imports. |
| src/pyarchrules/core/rules/linter/no_private_linter_rule.py | Adds TOML-driven linter rule for private imports. |
| src/pyarchrules/core/rules/linter/dependencies_rule.py | Tightens/expands internal dependency checking and wildcard support. |
| src/pyarchrules/core/rules/linter/allowed_service_dependencies_rule.py | Implements cross-service allowlist enforcement via import scanning. |
| src/pyarchrules/core/rules/linter/init.py | Exposes newly added linter rules. |
| src/pyarchrules/core/rules/dsl/tree_structure_rule.py | Adds a DSL tree-structure rule (currently duplicated vs existing implementation). |
| src/pyarchrules/core/rules/dsl/no_wildcard_imports_rule.py | Adds DSL rule for wildcard import detection. |
| src/pyarchrules/core/rules/dsl/no_test_files_in_rule.py | Adds DSL rule to forbid tests in production folders. |
| src/pyarchrules/core/rules/dsl/no_relative_imports_rule.py | Adds DSL rule to forbid relative imports. |
| src/pyarchrules/core/rules/dsl/no_private_imports_rule.py | Adds DSL rule to forbid private imports. |
| src/pyarchrules/core/rules/dsl/no_files_in_folder_rule.py | Adds DSL rule to enforce “folders only” structure. |
| src/pyarchrules/core/rules/dsl/no_circular_imports_rule.py | Adds DSL rule for circular import detection. |
| src/pyarchrules/core/rules/dsl/must_contain_folders_rule.py | Adds DSL must-contain-folders rule implementation. |
| src/pyarchrules/core/rules/dsl/must_contain_files_rule.py | Adds DSL must-contain-files rule implementation. |
| src/pyarchrules/core/rules/dsl/max_depth_rule.py | Adds DSL rule for maximum directory depth. |
| src/pyarchrules/core/rules/dsl/layer_must_not_import_rule.py | Adds DSL rule for forbidden layer imports. |
| src/pyarchrules/core/rules/dsl/forbidden_external_libs_rule.py | Adds DSL rule to forbid specified third-party imports. |
| src/pyarchrules/core/rules/dsl/files_must_match_pattern_rule.py | Adds DSL rule for folder file-name glob enforcement. |
| src/pyarchrules/core/rules/dsl/files_must_be_snake_case_rule.py | Adds DSL rule for snake_case module names. |
| src/pyarchrules/core/rules/dsl/classes_must_match_pattern_rule.py | Adds DSL rule for class-name regex enforcement. |
| src/pyarchrules/core/rules/dsl/allowed_external_libs_rule.py | Adds DSL rule to allowlist third-party imports. |
| src/pyarchrules/core/rules/dsl/init.py | Adds DSL package exports for the fluent API. |
| src/pyarchrules/core/rules/checks/naming.py | Adds AST-based naming helpers for DSL rules. |
| src/pyarchrules/core/rules/checks/imports.py | Adds shared import parsing helpers + cycle detection utilities. |
| src/pyarchrules/core/rules/checks/fs.py | Adds shared filesystem inspection helpers. |
| src/pyarchrules/core/rules/base/import_rule.py | Adds shared base class for import-scanning rules. |
| src/pyarchrules/core/rules/base/fs_rule.py | Adds shared base class for filesystem-scanning rules. |
| src/pyarchrules/core/rules/init.py | Updates core exports to match new structure. |
| src/pyarchrules/core/reporting.py | Improves reporter protocol/docs for results reporting. |
| src/pyarchrules/core/registries/linter_registry.py | Reworks linter registry to auto-load rules from ProjectSpec. |
| src/pyarchrules/core/registries/dsl_registry.py | Reworks DSL registry to pre-register rule sets per service. |
| src/pyarchrules/core/registries/base_registry.py | Introduces common validation/reporting/raise pipeline. |
| src/pyarchrules/core/registries/init.py | Exposes BaseRegistry alongside DSL/Linter registries. |
| src/pyarchrules/core/errors.py | Adds module/class docstring for PyArchError. |
| src/pyarchrules/core/config.py | Moves/updates config writer/reader and defaults (isolate_services). |
| src/pyarchrules/cli.py | Updates CLI to use new PyArchRules APIs and linter check execution. |
| src/pyarchrules/init.py | Adjusts loguru configuration and public exports. |
| pyproject.toml | Bumps version to beta, updates metadata and dev dependency format. |
| docs/mkdocs.yml | Adjusts navigation for docs site. |
| docs/docs/index.md | Updates landing page content and links; updates status badge. |
| docs/docs/getting-started.md | Updates quickstart flow and examples to new config options. |
| docs/docs/dsl.md | Expands DSL documentation and rule reference list. |
| docs/docs/configuration.md | Updates configuration reference for new project/service keys and behavior. |
| docs/docs/cli.md | Updates CLI reference and output examples. |
| README.md | Updates README for beta positioning, new config keys, and docs links. |
| Makefile | Switches formatting/linting to ruff-based workflow. |
| CONTRIBUTING.md | Adds contribution guidelines and repository structure overview. |
| CHANGELOG.md | Adds beta release entry and updates changelog table. |
Comments suppressed due to low confidence (3)
src/pyarchrules/core/rules/checks/naming.py:32
collect_class_names()claims to return “top-level” classes, but it usesast.walk(tree)which includes nestedClassDefs as well. Either update the docstring (e.g., “all class names”) or restrict the traversal totree.bodyto only collect top-level definitions.
def collect_class_names(path: Path) -> list[str]:
"""Return all top-level class names defined in a Python file.
Parameters
----------
path : Path
Path to a ``.py`` file.
Returns
-------
list[str]
Class name strings. Empty list on parse failure.
"""
try:
source = path.read_text(encoding="utf-8")
tree = ast.parse(source, filename=str(path))
except (SyntaxError, OSError):
return []
return [node.name for node in ast.walk(tree) if isinstance(node, ast.ClassDef)]
docs/docs/index.md:9
- The status badge version in the docs is
0.1.0b1, but the package version inpyproject.toml(and the README badge) is0.1.0b0. Align these to avoid publishing inconsistent version information.
<a href="https://github.com/mspitb/pyarchrules/blob/main/LICENSE"><img src="https://img.shields.io/badge/license-MIT-blue.svg" alt="License: MIT"></a>
<a href="https://www.python.org/"><img src="https://img.shields.io/badge/python-3.12+-blue.svg" alt="Python 3.12+"></a>
<img src="https://img.shields.io/badge/status-beta%200.1.0b1-orange.svg" alt="Status: Beta">
</p>
src/pyarchrules/core/config.py:125
_get_root()defaults to ".." which will resolve service paths relative to the parent directory whenrootis omitted. This contradicts initialization (which writesroot = ".") andSpecLoader(default root is "."), and can cause the CLI/config writer to generate configs that load differently than they display. Use "." as the default here for consistency.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+51
to
+58
| # Build a map of top-level folder name → service name for every OTHER service | ||
| other_services: dict[str, str] = {} | ||
| for svc_name, svc_spec in self._project_spec.services.items(): | ||
| if svc_name == self._service_spec.name: | ||
| continue | ||
| # The top-level import name is the first segment of the service path | ||
| top_level = Path(svc_spec.path).parts[0] if svc_spec.path != "." else svc_name | ||
| other_services[top_level] = svc_name |
There was a problem hiding this comment.
other_services is keyed by Path(svc_spec.path).parts[0], which breaks when services share a parent folder (e.g. services/api vs services/billing), because both keys become services and overwrite each other. This makes cross-service dependency enforcement unreliable. Use a non-colliding key (commonly the last path segment or the configured service name) or validate that service paths have unique import roots.
Comment on lines
+32
to
+38
| tree = self._service_spec.tree | ||
| if not tree: | ||
| # No tree specification, nothing to validate | ||
| return violations | ||
|
|
||
| for path, node_spec in tree.items(): | ||
| violations.extend(self._validate_node(service_dir, path, node_spec)) |
There was a problem hiding this comment.
ServiceSpec.tree is defined as list[str], but this rule treats it as a mapping (for path, node_spec in tree.items()). As written, validate() will raise at runtime when tree is configured via TOML (a list). Either change ServiceSpec.tree to the expected structure or update this rule to iterate over a list of paths (similar to TreeRule).
|
|
||
| class LinterRegistry: | ||
| """Registry for storing linter rules per service.""" | ||
| class LinterRegistry(BaseRegistry["list[Rule]"]): |
There was a problem hiding this comment.
LinterRegistry subclasses BaseRegistry["list[Rule]"] using a string inside the generic subscription. This isn’t a valid type parameter (and will confuse type checkers / IDEs); use BaseRegistry[list[Rule]] (or BaseRegistry[list["Rule"]] if you truly need a forward reference).
Suggested change
| class LinterRegistry(BaseRegistry["list[Rule]"]): | |
| class LinterRegistry(BaseRegistry[list["Rule"]]): |
Comment on lines
35
to
+51
| @@ -34,79 +46,73 @@ def project_spec(self) -> ProjectSpec: | |||
| return self._project_spec | |||
|
|
|||
| @property | |||
| def dsl_registry(self) -> DSLRegistry: | |||
| return self._dsl_registry | |||
| def services(self) -> dict[str, Path]: | |||
| """Return a mapping of service names to their paths.""" | |||
| return self._services | |||
There was a problem hiding this comment.
services is annotated as dict[str, Path], but _services is built from spec.path which is a str (relative path). Either change the return type to dict[str, str] or convert the values to Path (e.g., Path(spec.path) or spec.absolute_path) to match the annotation and docstring.
Comment on lines
74
to
85
| ```bash | ||
| pyarchrules check [PROJECT_ROOT] [--strict/--no-strict] [--verbose/--quiet] | ||
| pyarchrules check [PROJECT_ROOT] [--verbose | --quiet] | ||
| ``` | ||
|
|
||
| Validates architecture against all rules in `pyproject.toml`. | ||
| Validates the project against all rules defined in `pyproject.toml`. | ||
|
|
||
| | Option | Default | Description | | ||
| |--------|---------|-------------| | ||
| | `--strict` / `--no-strict` | from config | Override strict mode. | | ||
| | `--verbose` / `--quiet` | `--verbose` | Show per-service rule detail. | | ||
| | `PROJECT_ROOT` | `.` | Path to the project root directory. | | ||
| | `--verbose` | on | Show per-service rule detail. | | ||
| | `--quiet` | — | Suppress per-service output. | | ||
|
|
There was a problem hiding this comment.
The CLI docs for check don’t mention the --strict/--no-strict option, but src/pyarchrules/cli.py still supports it and can exit 0 on violations when --no-strict is used. Either document this flag here (and adjust the exit-code table), or remove/rename the flag so the documented behavior matches the implementation.
Comment on lines
+50
to
+58
| # Build map: top-level folder → service name, only for non-shared services | ||
| forbidden_services: dict[str, str] = {} | ||
| for svc_name, svc_spec in self._project_spec.services.items(): | ||
| if svc_name == self._service_spec.name: | ||
| continue | ||
| if svc_spec.shared: | ||
| continue # shared services may be imported freely | ||
| top_level = Path(svc_spec.path).parts[0] if svc_spec.path != "." else svc_name | ||
| forbidden_services[top_level] = svc_name |
There was a problem hiding this comment.
Mapping sibling services by Path(svc_spec.path).parts[0] will collide when services are nested under a common parent (e.g. services/api and services/billing both map to services). That makes the isolation check inaccurate (last one wins) and can miss/false-flag imports. Consider mapping by the service’s actual import root (often the last path segment) or explicitly documenting/enforcing that service path must be top-level and unique.
Comment on lines
+21
to
+23
| ``source -> target`` means *source* is allowed to import from *target*. | ||
| Modules whose source folder is **not** covered by any rule are unrestricted. | ||
| Modules whose source folder **is** covered may only import from the listed targets. |
There was a problem hiding this comment.
The DependenciesRule docstring says uncovered source folders are unrestricted, but the current _is_import_allowed() returns False when no source rule matches (making imports from uncovered sources forbidden). Either adjust the implementation to allow uncovered sources, or update the docstring to match the new “explicit allowlist” semantics.
Comment on lines
+1
to
+12
| """Tree structure validation rule.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from pyarchrules.core.rules.rule import Rule | ||
| from pyarchrules.model.rules.rule_violation import RuleViolation | ||
| from pyarchrules.model.spec.service_spec import ServiceSpec | ||
|
|
||
|
|
||
| class TreeStructureRule(Rule): | ||
| """Rule that validates directory tree structure against specification.""" | ||
|
|
There was a problem hiding this comment.
This TreeStructureRule duplicates the existing implementation at src/pyarchrules/core/rules/tree_structure_rule.py (same class name and rule_name). Having two copies increases maintenance cost and creates import ambiguity. Consider keeping a single implementation and re-exporting it from the desired package path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update mkdocs, fix rules issues, update unit tests