Conversation
Update mkdocs, fix rules issues, update unit tests
There was a problem hiding this comment.
Pull request overview
This is a beta release preparation (0.1.0b0) that introduces significant architectural improvements, new DSL rules, comprehensive documentation, and dependency rule enhancements.
Changes:
- Major restructuring of rule system with new base classes (ImportBaseRule, FsBaseRule) and extensive DSL rules
- Enhanced dependency validation with wildcard support and bidirectional enforcement
- New service isolation feature with shared service support
- Complete documentation site with MkDocs Material
Reviewed changes
Copilot reviewed 81 out of 85 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Version bump to 0.1.0b0, status to Beta, updated URLs and dependencies |
| src/pyarchrules/core/spec_loader.py | Added key validation, root path resolution, and new service spec fields |
| src/pyarchrules/model/spec/project_spec.py | Replaced strict/fail_on_warning with isolate_services flag |
| src/pyarchrules/model/spec/service_spec.py | Added no_wildcard_imports, no_private_imports, shared fields with documentation |
| src/pyarchrules/core/rules/linter/dependencies_rule.py | Enhanced with wildcard rules, path validation, and improved import resolution |
| src/pyarchrules/core/rules/linter/service_isolation_rule.py | New rule enforcing service boundaries with shared flag support |
| src/pyarchrules/core/rules/dsl/*.py | 13 new DSL rules for comprehensive architecture validation |
| src/pyarchrules/core/registries/*.py | Refactored with BaseRegistry, auto-loading from spec |
| src/pyarchrules/pyarchrules.py | Redesigned API with separate validate() and check_linter() methods |
| docs/** | Complete documentation site with getting started, configuration, CLI reference, and DSL guide |
| tests/unit/dsl/*.py | Comprehensive test coverage for all new DSL rules |
Comments suppressed due to low confidence (1)
src/pyarchrules/core/rules/linter/dependencies_rule.py:1
- For absolute imports, only the root package should be returned (the first segment), not the full path. The current logic replaces all dots, which would incorrectly treat 'domain.models.user' as a path 'domain/models/user' when checking against rules defined for top-level packages like 'domain'. This breaks the folder-based matching logic used in _is_import_allowed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 85 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/pyarchrules/core/config.py:125
- The default value for
rootin_get_root()should be"."not"..". Line 125 returns".."as the default which would point to the parent directory of the project root, but the default should be the project root itself (same directory as pyproject.toml). This is inconsistent with the documentation and theinitialize()method which correctly setsroot = "."on line 55.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 85 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/pyarchrules/core/config.py:125
_get_root()defaults to ".." whenrootis missing, which would make service paths resolve outside the project by default and contradictsinitialize()(which writesroot = ".") andSpecLoader(default root is "."). Defaulting to ".." looks unintended; change the fallback to "." (or remove the fallback and require the key).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 85 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/pyarchrules/core/config.py:125
- The default value for
_get_root()is changed from"."to".."on line 125. This appears to be a bug - the documentation and tests consistently show the default should be".", not"..".
tests/unit/test_spec_loader.py:67 - Two tests for the
strictconfiguration option have been removed:test_default_strict_is_trueandtest_parses_strict_false. This aligns with the removal of thestrictfield fromProjectSpec(replaced withisolate_services). However, the removal means there's no test coverage for default values of the newisolate_servicesfield. Consider adding a test liketest_default_isolate_services_is_falseto ensure the default is properly validated.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -82,7 +85,8 @@ def test_unspecified_imports_allowed(self, make_service_spec, tmp_test_dir, monk | |||
| violations = DependenciesRule(spec).validate() | |||
|
|
|||
| import_errors = [v for v in violations if "Forbidden import" in v.message] | |||
| assert len(import_errors) == 0 | |||
| assert len(import_errors) == 1 | |||
| assert "domain" in import_errors[0].details["from_module"] | |||
There was a problem hiding this comment.
This test represents a significant behavior change. Previously, when only api -> domain was declared, imports from domain to api were allowed (the old test name was test_unspecified_imports_allowed). Now they are forbidden. This changes the semantics of the dependencies configuration from a permissive allowlist (only specified directions are validated) to a strict whitelist (any covered source can only import from explicitly listed targets).
While this may be intentional, it's a breaking change that should be clearly documented. Users who had partial dependency rules expecting uncovered directions to be allowed will now see violations.
| 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.
The services attribute has been changed from a public attribute to a property. In line 35 it's stored as self._services (private), and then exposed via a @property decorator on line 49. While this is better encapsulation, it's a breaking change for any code that was setting self.services directly. The change should be documented in release notes.
| def _collect_violations(self) -> list: | ||
| violations = [] | ||
| for rule_set in self._store.values(): | ||
| violations.extend(rule_set._collect_violations()) # noqa |
There was a problem hiding this comment.
Line 34 accesses rule_set._collect_violations() which is a private method (indicated by the leading underscore and the # noqa comment to suppress linting). This violates encapsulation. Consider making _collect_violations() a public method in ServiceRuleSet or adding a proper public API like validate() that returns violations.
No description provided.