refactor: replace SubProject inheritance with has-a Fetcher composition#1170
Open
spoorcc wants to merge 6 commits into
Open
refactor: replace SubProject inheritance with has-a Fetcher composition#1170spoorcc wants to merge 6 commits into
spoorcc wants to merge 6 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors SubProject from an abstract base with VCS-specific subclasses into a concrete orchestrator that delegates to injected Fetcher implementations. Adds Fetcher/VcsFetcher/AbstractVcsFetcher protocols and implementations (GitFetcher, SvnFetcher, ArchiveFetcher). Updates commands (add, format_patch, environment), tests, and UI to use the new ChangesFetcher Protocol & Core Refactor
Concrete Fetcher Implementations
Command and UI Integration
Infrastructure, Tests, and Documentation
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant SubProject
participant Fetcher
participant Remote as Remote VCS/Archive
User->>SubProject: update() / check_for_update() / browse tree / freeze
SubProject->>Fetcher: fetch() / wanted_version() / as_vcs() / latest_available_version()
Fetcher->>Remote: VCS ops (list branches/tags, clone/export/download, tree ls, check revision)
Remote-->>Fetcher: refs / data / tree entries / hash
Fetcher-->>SubProject: (Version, [Dependency]) / Version / VcsFetcher / Version | None
SubProject->>Fetcher: freeze(project, on_disk_version)
Fetcher-->>SubProject: pinned hash/revision string | None
SubProject-->>User: success / metadata updated / dependencies discovered
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_subproject.py (1)
175-196: 🧹 Nitpick | 🔵 TrivialConsider reducing context manager nesting with
contextlib.ExitStack.Six levels of nested
withstatements reduce readability. UsingExitStackorpytest-mock'smockerfixture would flatten this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_subproject.py` around lines 175 - 196, Replace the six nested with-patch context managers in the test with a single contextlib.ExitStack to improve readability: import ExitStack and use "with ExitStack() as stack:" then enter each patch via stack.enter_context(patch("dfetch.project.subproject.os.path.exists")) (and likewise for "dfetch.project.subproject.Metadata.from_file", "dfetch.project.subproject.hash_directory", "dfetch.project.subproject.safe_rm", "dfetch.project.metadata.Metadata.fetched", "dfetch.project.metadata.Metadata.dump"), keep the same mock return values and the rest of the test exercising _make_subproject(...).update(...) and assertions unchanged so mock_hash.call_args still inspects the "skiplist".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/project/__init__.py`:
- Line 41: The RuntimeError raised ("vcs type unsupported") should include the
remote URL to aid debugging; update the raise in dfetch.project.__init__.py to
interpolate the remote URL variable in scope (e.g., url, remote_url, or remote)
into the error message and provide brief context like "vcs type unsupported for
<remote_url>" so developers can see which manifest entry failed when the
exception is raised.
In `@dfetch/project/gitsubproject.py`:
- Around line 41-44: The inline "# type: ignore" on the `@lru_cache` decorator of
get_default_branch hides a typing/design problem: lru_cache on an instance
method shares cache across instances. Replace the decorator with
functools.cached_property (or functools.cached_property if backport present) on
the get_default_branch method so the default branch is computed and cached
per-instance, or if you intentionally want a cross-instance cache, remove the
type-ignore and add a clear docstring on get_default_branch describing the
shared-cache behavior and change `@lru_cache` to `@functools.lru_cache`(maxsize=...)
to make intent explicit; update imports to use functools.cached_property or
functools.lru_cache accordingly and remove the "# type: ignore" comment.
In `@pyproject.toml`:
- Around line 158-160: The two functions currently using inline "# pylint:
disable" waivers (the add-command handler in add.py around the complaint at line
~304 and the pick helper in pick.py around line ~77) exceed the configured
max-args; remove the inline suppressions and refactor those functions to accept
≤6 parameters by grouping related parameters into small dataclasses/objects or a
single context/config object, breaking large functions into smaller helpers that
take fewer arguments, or moving optional args into keyword-only config objects;
after refactoring, delete the inline "# pylint: disable" comments so the
functions conform to the pyproject.toml design thresholds.
In `@tests/test_subproject.py`:
- Around line 68-72: The factory function _make_subproject has an imprecise type
for the wanted parameter; update its signature to explicitly allow None by
changing wanted: Version = None to wanted: Version | None = None (or
Optional[Version] if using typing imports) so the function return typing is
accurate; adjust any related annotations/tests that rely on this signature and
ensure Version is imported/available in the scope where _make_subproject is
defined.
- Around line 24-25: Update the __init__ signature to explicitly annotate the
optional parameter as Version | None instead of just using a default of None;
locate the __init__ method on the class in tests/test_subproject.py (the method
that sets self._wanted) and change the parameter annotation from wanted: Version
= None to wanted: Version | None so static type checkers recognize None is
allowed, leaving the default and internal assignment (self._wanted = wanted or
Version()) unchanged.
---
Outside diff comments:
In `@tests/test_subproject.py`:
- Around line 175-196: Replace the six nested with-patch context managers in the
test with a single contextlib.ExitStack to improve readability: import ExitStack
and use "with ExitStack() as stack:" then enter each patch via
stack.enter_context(patch("dfetch.project.subproject.os.path.exists")) (and
likewise for "dfetch.project.subproject.Metadata.from_file",
"dfetch.project.subproject.hash_directory", "dfetch.project.subproject.safe_rm",
"dfetch.project.metadata.Metadata.fetched",
"dfetch.project.metadata.Metadata.dump"), keep the same mock return values and
the rest of the test exercising _make_subproject(...).update(...) and assertions
unchanged so mock_hash.call_args still inspects the "skiplist".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f45e5c4-05b0-46e3-9d5d-ca89a93beda2
📒 Files selected for processing (16)
dfetch/commands/add.pydfetch/commands/environment.pydfetch/commands/format_patch.pydfetch/project/__init__.pydfetch/project/archivesubproject.pydfetch/project/fetcher.pydfetch/project/gitsubproject.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/project/svnsubproject.pydfetch/project/svnsuperproject.pypyproject.tomltests/test_add.pytests/test_archive.pytests/test_subproject.pytests/test_svn.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/commands/add.py`:
- Around line 275-285: The resolver _resolve_entry_version should branch on
ctx.subproject.as_vcs() and avoid using VCS branch/tag lookup for archive-backed
subprojects; update _resolve_entry_version (and the similar logic around lines
290-294) so that if ctx.subproject.as_vcs() is True you use the existing
branches/tags + _resolve_raw_version behavior, but if False you either parse the
raw_version with archive-specific logic provided by the subproject (e.g., an
archive-version parser on ctx.subproject) or explicitly reject/raise for
unsupported --version inputs, returning a Version that preserves the archive
identifier (not a branch: field) when accepting archive versions. Ensure you
reference Version and _resolve_raw_version only for VCS subprojects and do not
fall back to Version(branch=raw_version) for archives.
In `@dfetch/log.py`:
- Around line 21-30: Remove the inline "# pylint:
disable=too-few-public-methods" comments by replacing the small adapter classes
with a function-based wrapper around the existing LogRender type: remove class
_LogRenderBase and its inline suppression and instead provide a factory function
(e.g., make_log_renderer or wrap_log_render) that returns a callable
implementing the same behavior as _LogRenderBase.__init__ and
_LogRenderBase.__call__, and replace _NoExpandLogRender class usage with a
function that returns the adapted LogRender callable; ensure references to
_LogRenderBase, _NoExpandLogRender, __init__, and __call__ are updated to use
the new wrapper so no inline pylint suppression is required.
In `@dfetch/project/archivesubproject.py`:
- Around line 101-125: The fetch() path currently uses the original URL as the
returned Version for non-hash archives, allowing a later freeze() to re-download
a possibly-changed remote; change fetch() to capture the actual downloaded
content digest from the download/verify step and return a Version pinned to that
digest so the on-disk tree matches the recorded pin. Concretely: make
_download_and_verify (or the helper it calls) return the computed digest (or
expose it on the Archivesubproject instance), then in fetch() call that,
build/replace the returned Version.revision with the computed digest (instead of
leaving the URL), and return that updated Version; apply the same change to the
analogous code path referenced for lines 141-161 so both fetch code paths record
and return the fetched content digest rather than the original remote URL.
- Around line 97-99: latest_available_version currently returns wanted if the
archive URL is reachable, which wrongly reports hash-pinned archives as
available; change latest_available_version so that when an integrity.hash is
present for the project (the integrity field / integrity.hash), it does not rely
on is_accessible() alone but verifies the remote bytes match the pinned digest
before returning wanted — e.g. call a verification routine (download/stream and
compute the same digest, or use a remote-repo verify method if available) and
only return wanted when the computed digest equals integrity.hash, otherwise
return None (and treat transient errors as not-available); this prevents callers
(and fetch()) from being misled by reachability alone.
In `@dfetch/project/fetcher.py`:
- Around line 126-134: latest_available_version currently collapses
revision-only pins into a branch lookup (trunk) which breaks SVN pinned
revisions; update the logic so revision-only requests are preserved: either
modify latest_available_version (the method shown) to return
self._revision_version_if_exists(wanted.revision) when
self._is_revision_only(wanted) without depending on revision_is_enough(), or
implement an override in SvnFetcher (create SvnFetcher.latest_available_version)
that checks _is_revision_only and returns the specific revision via
_revision_version_if_exists; reference _is_revision_only,
_revision_version_if_exists, latest_available_version and SvnFetcher.fetch to
ensure SvnFetcher continues to accept Version(revision=...) unchanged.
In `@tests/test_subproject.py`:
- Around line 50-51: MockVcsFetcher.browse_tree currently returns None, so tests
that call with sp.as_vcs().browse_tree(...) fail; change
MockVcsFetcher.browse_tree to return a real context manager (e.g., use
contextlib.contextmanager or contextlib.nullcontext) that yields an appropriate
dummy tree/temporary path object expected by consumers; ensure the returned
context manager implements __enter__/__exit__ and yields the same type/shape
that SubProject.as_vcs() consumers expect so tests using with
sp.as_vcs().browse_tree(...) work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a968269-6a6f-4270-bdbc-37c18720535d
📒 Files selected for processing (11)
dfetch/commands/add.pydfetch/log.pydfetch/project/archivesubproject.pydfetch/project/fetcher.pydfetch/project/gitsubproject.pydfetch/project/subproject.pydfetch/project/svnsubproject.pydfetch/terminal/pick.pydfetch/terminal/prompt.pydfetch/util/license.pytests/test_subproject.py
Comment on lines
+97
to
+99
| def latest_available_version(self, wanted: Version) -> Version | None: | ||
| """Return *wanted* if the archive URL is still reachable, else None.""" | ||
| return wanted if self._remote_repo.is_accessible() else None |
Contributor
There was a problem hiding this comment.
Don't report hash-pinned archives as available on reachability alone.
For integrity.hash entries this returns wanted whenever the URL responds, even if the bytes at that URL no longer match the pinned digest. fetch() will then fail with a hash mismatch, so callers can treat an unfetchable version as available.
Suggested fix
def latest_available_version(self, wanted: Version) -> Version | None:
"""Return *wanted* if the archive URL is still reachable, else None."""
- return wanted if self._remote_repo.is_accessible() else None
+ expected = IntegrityHash.parse(wanted.revision)
+ if expected is None:
+ return wanted if self._remote_repo.is_accessible() else None
+
+ with temp_file(_suffix_for_url(self._remote)) as tmp_path:
+ actual_hex = self._remote_repo.download(
+ tmp_path, algorithm=expected.algorithm
+ )
+ return wanted if expected.matches(actual_hex) else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/archivesubproject.py` around lines 97 - 99,
latest_available_version currently returns wanted if the archive URL is
reachable, which wrongly reports hash-pinned archives as available; change
latest_available_version so that when an integrity.hash is present for the
project (the integrity field / integrity.hash), it does not rely on
is_accessible() alone but verifies the remote bytes match the pinned digest
before returning wanted — e.g. call a verification routine (download/stream and
compute the same digest, or use a remote-repo verify method if available) and
only return wanted when the computed digest equals integrity.hash, otherwise
return None (and treat transient errors as not-available); this prevents callers
(and fetch()) from being misled by reachability alone.
Comment on lines
+101
to
125
| def fetch( | ||
| self, | ||
| version: Version, | ||
| local_path: str, | ||
| name: str, | ||
| source: str, | ||
| ignore: Sequence[str], | ||
| ) -> tuple[Version, list[Dependency]]: | ||
| """Download and extract the archive to *local_path*. | ||
|
|
||
| Raises: | ||
| RuntimeError: On download failure or hash mismatch. | ||
|
|
||
| Returns: | ||
| The version that was actually fetched (hash string or URL). | ||
| """ | ||
| revision = version.revision | ||
|
|
||
| pathlib.Path(self.local_path).mkdir(parents=True, exist_ok=True) | ||
|
|
||
| with temp_file(_suffix_for_url(self.remote)) as tmp_path: | ||
| expected = IntegrityHash.parse(revision) | ||
| if expected: | ||
| actual_hex = self._remote_repo.download( | ||
| tmp_path, algorithm=expected.algorithm | ||
| ) | ||
| if not expected.matches(actual_hex): | ||
| raise RuntimeError( | ||
| f"Hash mismatch for {self._project_entry.name}! " | ||
| f"{expected.algorithm} expected {expected.hex_digest}" | ||
| ) | ||
| else: | ||
| self._remote_repo.download(tmp_path) | ||
| pathlib.Path(local_path).mkdir(parents=True, exist_ok=True) | ||
|
|
||
| with temp_file(_suffix_for_url(self._remote)) as tmp_path: | ||
| self._download_and_verify(version.revision, tmp_path, name) | ||
| ArchiveLocalRepo.extract( | ||
| tmp_path, | ||
| self.local_path, | ||
| src=self.source, | ||
| ignore=self.ignore, | ||
| local_path, | ||
| src=source, | ||
| ignore=ignore, | ||
| ) | ||
|
|
||
| return version, [] |
Contributor
There was a problem hiding this comment.
Freeze can pin different bytes than the tree you just fetched.
For non-hash archives, fetch() returns the original URL as the on-disk version, and freeze() later re-downloads that URL to compute the pin. If the remote changes between those two steps, the manifest gets pinned to a different archive than the one extracted locally. Record the fetched content digest during fetch() and reuse that here instead of re-fetching.
Also applies to: 141-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/archivesubproject.py` around lines 101 - 125, The fetch() path
currently uses the original URL as the returned Version for non-hash archives,
allowing a later freeze() to re-download a possibly-changed remote; change
fetch() to capture the actual downloaded content digest from the download/verify
step and return a Version pinned to that digest so the on-disk tree matches the
recorded pin. Concretely: make _download_and_verify (or the helper it calls)
return the computed digest (or expose it on the Archivesubproject instance),
then in fetch() call that, build/replace the returned Version.revision with the
computed digest (instead of leaving the URL), and return that updated Version;
apply the same change to the analogous code path referenced for lines 141-161 so
both fetch code paths record and return the fetched content digest rather than
the original remote URL.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dfetch/commands/add.py (1)
87-108:⚠️ Potential issue | 🟠 MajorFall back to text prompts when the subproject has no VCS browser.
For archive-backed subprojects this branch yields an
LsFunctionthat always returns[], but_ask_src()and_ask_ignore()still take the TTY tree-browser path. Indfetch add -i, users can no longer entersrc:orignore:values for archives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/add.py` around lines 87 - 108, The fallback currently yields an empty LsFunction (_empty) when subproject.as_vcs() is None, which causes the TTY tree-browser path to run and prevents _ask_src()/_ask_ignore() from falling back to text prompts; change the else branch to yield None (remove or stop using _empty) so callers detect "no VCS browser" and _ask_src()/_ask_ignore() can present the text prompt path (affecting dfetch add -i).
♻️ Duplicate comments (2)
dfetch/log.py (1)
67-70:⚠️ Potential issue | 🟠 MajorRemove inline pylint suppression at Line 70.
# pylint: disable=protected-accessindfetch/log.pyviolates the repository lint policy fordfetch/**/*.py. Please remove the inline suppression and route this through a small helper (or local adapter) so lint passes without suppression.Proposed fix (no inline suppression)
+def _set_handler_log_render(handler: RichHandler, render: Any) -> None: + """Install a custom Rich log renderer on a handler.""" + setattr(handler, "_log_render", render) + + def configure_root_logger(console: Console | None = None) -> None: @@ if os.getenv("ASCIINEMA_REC"): no_expand = _make_non_expanding_log_render( show_time=False, show_level=False, show_path=False ) - handler._log_render = no_expand # pylint: disable=protected-access + _set_handler_log_render(handler, no_expand)As per coding guidelines: "Do not use inline lint suppressions (# noqa, # type: ignore, # pylint: disable, # pyright: ignore) without fixing the root cause. Only module-level tool headers in test files (lines 1-5) are permitted."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/log.py` around lines 67 - 70, Remove the inline pylint suppression and instead encapsulate the protected attribute assignment in a small helper so linting is satisfied; create a helper function (e.g., set_handler_log_render(handler, render) or a local adapter) in dfetch/log.py that sets handler._log_render = render (or uses setattr) and call that helper where you currently call _make_non_expanding_log_render, replacing the direct protected-access line (handler._log_render = no_expand) with a call to the new helper; keep the rest of the logic unchanged and do not add any inline pylint disables.dfetch/commands/add.py (1)
275-291:⚠️ Potential issue | 🟠 MajorReuse
_resolve_entry_version()from the interactive--versionpath too.This helper fixes archive
--versionhandling only for_non_interactive_entry()._interactive_flow()still resolves overrides inline, sodfetch add -i <archive> --version foofalls back toVersion(branch="foo")instead of preserving the archive revision.Suggested follow-up in `_interactive_flow()`
if overrides.version is not None: - choices: list[Version] = [ - *[ - Version(branch=b) - for b in prioritise_default(branches, ctx.default_branch) - ], - *[Version(tag=t) for t in sort_tags_newest_first(tags)], - ] - version = _resolve_raw_version(overrides.version, choices) or Version( - branch=ctx.default_branch - ) + version = _resolve_entry_version(ctx, overrides.version) else: version = _ask_version(ctx.default_branch, branches, tags)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/add.py` around lines 275 - 291, Interactive flow currently resolves version overrides inline and doesn't use _resolve_entry_version, causing archive --version values to be treated as branches; update _interactive_flow to call _resolve_entry_version(ctx, raw_version) wherever it currently constructs Version(branch=raw_version) or resolves raw_version manually (compare to _non_interactive_entry usage) so archives preserve revision identifiers; ensure you pass the same _AddContext (ctx) and replace inline Version(...) creation with the returned Version object from _resolve_entry_version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/project/svnsubproject.py`:
- Around line 143-152: The _copy_license_files method only exports the first
detected file (license_files[0]) so additional files like NOTICE or COPYING are
skipped; update _copy_license_files to iterate over all entries returned by
SvnFetcher._license_files(root_branch_path) and call SvnRepo.export for each
file (use the same root_branch_path, revision and computed dest), ensuring dest
is computed once from local_path and used for every export.
- Around line 132-157: Add Google-style docstrings to the three private helpers
so pydocstyle is satisfied: for _apply_file_pattern(local_path, file_pattern,
source) add a short summary stating it removes files that don't match
file_pattern and warns if the directory is empty, then document Args
(local_path, file_pattern, source), Returns (None) and any side effects (deletes
files, logs warning); for _copy_license_files(local_path, branch_path, revision)
add a summary that it finds license files in the remote branch and exports the
first match into dest, then document Args (local_path, branch_path, revision),
Returns (None) and side effects (calls SvnRepo.export); for
_remove_ignored_files(local_path, ignore) add a summary that it removes matched
files/dirs except recognized license files, document Args (local_path, ignore),
Returns (None) and side effects (calls safe_rm), and include any raised
exceptions if applicable.
- Around line 69-79: In latest_available_version, don't assume any bare revision
pin is valid—validate the revision format (must be a numeric SVN revision)
before returning a Version; if wanted.revision is not an integer (e.g. contains
letters like "abc") return None instead of Version(...). Update the branch that
checks wanted.revision and not wanted.branch and not wanted.tag to
parse/regex-validate wanted.revision and only call
Version(revision=wanted.revision, branch=self.get_default_branch()) when
validation succeeds so callers won't treat invalid pins as available (fetch()
later rejects invalid revisions).
---
Outside diff comments:
In `@dfetch/commands/add.py`:
- Around line 87-108: The fallback currently yields an empty LsFunction (_empty)
when subproject.as_vcs() is None, which causes the TTY tree-browser path to run
and prevents _ask_src()/_ask_ignore() from falling back to text prompts; change
the else branch to yield None (remove or stop using _empty) so callers detect
"no VCS browser" and _ask_src()/_ask_ignore() can present the text prompt path
(affecting dfetch add -i).
---
Duplicate comments:
In `@dfetch/commands/add.py`:
- Around line 275-291: Interactive flow currently resolves version overrides
inline and doesn't use _resolve_entry_version, causing archive --version values
to be treated as branches; update _interactive_flow to call
_resolve_entry_version(ctx, raw_version) wherever it currently constructs
Version(branch=raw_version) or resolves raw_version manually (compare to
_non_interactive_entry usage) so archives preserve revision identifiers; ensure
you pass the same _AddContext (ctx) and replace inline Version(...) creation
with the returned Version object from _resolve_entry_version.
In `@dfetch/log.py`:
- Around line 67-70: Remove the inline pylint suppression and instead
encapsulate the protected attribute assignment in a small helper so linting is
satisfied; create a helper function (e.g., set_handler_log_render(handler,
render) or a local adapter) in dfetch/log.py that sets handler._log_render =
render (or uses setattr) and call that helper where you currently call
_make_non_expanding_log_render, replacing the direct protected-access line
(handler._log_render = no_expand) with a call to the new helper; keep the rest
of the logic unchanged and do not add any inline pylint disables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45636da9-cf90-4abf-af75-cec9161d7648
📒 Files selected for processing (4)
dfetch/commands/add.pydfetch/log.pydfetch/project/svnsubproject.pytests/test_subproject.py
Comment on lines
+69
to
+79
| def latest_available_version(self, wanted: Version) -> Version | None: | ||
| """Return the latest version matching *wanted*, or None if unavailable. | ||
|
|
||
| @staticmethod | ||
| def list_tool_info() -> None: | ||
| """Print out version information.""" | ||
| try: | ||
| tool, version = get_svn_version() | ||
| SubProject._log_tool(tool, version) | ||
| except RuntimeError as exc: | ||
| logger.debug( | ||
| f"Something went wrong trying to get the version of svn: {exc}" | ||
| ) | ||
| SubProject._log_tool("svn", "<not found in PATH>") | ||
| For revision-only pins (no branch, no tag) the pinned revision is | ||
| returned with the default branch so version comparison works correctly. | ||
| SVN revisions are globally ordered within a repository, so a bare | ||
| ``revision:`` in the manifest is always relative to trunk. | ||
| """ | ||
| if wanted.revision and not wanted.branch and not wanted.tag: | ||
| return Version(revision=wanted.revision, branch=self.get_default_branch()) | ||
| return super().latest_available_version(wanted) |
Contributor
There was a problem hiding this comment.
Validate bare SVN revisions before treating them as available.
Line 77 now returns a Version for any revision-only pin, including values like revision: abc that fetch() rejects later at Line 179. Invalid pins should return None here, otherwise callers can treat them as current until the later export step blows up.
Suggested fix
def latest_available_version(self, wanted: Version) -> Version | None:
"""Return the latest version matching *wanted*, or None if unavailable.
@@
SVN revisions are globally ordered within a repository, so a bare
``revision:`` in the manifest is always relative to trunk.
"""
if wanted.revision and not wanted.branch and not wanted.tag:
- return Version(revision=wanted.revision, branch=self.get_default_branch())
+ if not wanted.revision.isdigit():
+ return None
+ return Version(
+ revision=wanted.revision,
+ branch=self.get_default_branch(),
+ )
return super().latest_available_version(wanted)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/svnsubproject.py` around lines 69 - 79, In
latest_available_version, don't assume any bare revision pin is valid—validate
the revision format (must be a numeric SVN revision) before returning a Version;
if wanted.revision is not an integer (e.g. contains letters like "abc") return
None instead of Version(...). Update the branch that checks wanted.revision and
not wanted.branch and not wanted.tag to parse/regex-validate wanted.revision and
only call Version(revision=wanted.revision, branch=self.get_default_branch())
when validation succeeds so callers won't treat invalid pins as available
(fetch() later rejects invalid revisions).
Comment on lines
+132
to
+157
| def _apply_file_pattern( | ||
| self, local_path: str, file_pattern: str, source: str | ||
| ) -> None: | ||
| for file in find_non_matching_files(local_path, (file_pattern,)): | ||
| os.remove(file) | ||
| if not os.listdir(local_path): | ||
| logger.warning( | ||
| f"The 'src:' filter '{source}' didn't match any files" | ||
| f" from '{self._remote}'" | ||
| ) | ||
|
|
||
| complete_path = "/".join( | ||
| filter(None, [self.remote, branch_path.strip(), self.source]) | ||
| ).strip("/") | ||
| def _copy_license_files( | ||
| self, local_path: str, branch_path: str, revision: str | ||
| ) -> None: | ||
| root_branch_path = "/".join([self._remote, branch_path]).strip("/") | ||
| license_files = SvnFetcher._license_files(root_branch_path) | ||
| if license_files: | ||
| dest = ( | ||
| local_path if os.path.isdir(local_path) else os.path.dirname(local_path) | ||
| ) | ||
| SvnRepo.export(f"{root_branch_path}/{license_files[0]}", revision, dest) | ||
|
|
||
| # When exporting a file, the destination directory must already exist | ||
| pathlib.Path(os.path.dirname(self.local_path)).mkdir( | ||
| parents=True, exist_ok=True | ||
| ) | ||
| def _remove_ignored_files(self, local_path: str, ignore: Sequence[str]) -> None: | ||
| for file_or_dir in find_matching_files(local_path, ignore): | ||
| if not (file_or_dir.is_file() and is_license_file(file_or_dir.name)): | ||
| safe_rm(file_or_dir) |
Contributor
There was a problem hiding this comment.
Add docstrings to the new private helpers.
_apply_file_pattern(), _copy_license_files(), and _remove_ignored_files() are in dfetch/ but currently have no docstrings, so this refactor will keep pydocstyle complaining about the module.
As per coding guidelines, "Follow Google-style docstring conventions, enforced by pydocstyle on dfetch/ (but not on tests/)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/svnsubproject.py` around lines 132 - 157, Add Google-style
docstrings to the three private helpers so pydocstyle is satisfied: for
_apply_file_pattern(local_path, file_pattern, source) add a short summary
stating it removes files that don't match file_pattern and warns if the
directory is empty, then document Args (local_path, file_pattern, source),
Returns (None) and any side effects (deletes files, logs warning); for
_copy_license_files(local_path, branch_path, revision) add a summary that it
finds license files in the remote branch and exports the first match into dest,
then document Args (local_path, branch_path, revision), Returns (None) and side
effects (calls SvnRepo.export); for _remove_ignored_files(local_path, ignore)
add a summary that it removes matched files/dirs except recognized license
files, document Args (local_path, ignore), Returns (None) and side effects
(calls safe_rm), and include any raised exceptions if applicable.
Comment on lines
+143
to
+152
| def _copy_license_files( | ||
| self, local_path: str, branch_path: str, revision: str | ||
| ) -> None: | ||
| root_branch_path = "/".join([self._remote, branch_path]).strip("/") | ||
| license_files = SvnFetcher._license_files(root_branch_path) | ||
| if license_files: | ||
| dest = ( | ||
| local_path if os.path.isdir(local_path) else os.path.dirname(local_path) | ||
| ) | ||
| SvnRepo.export(f"{root_branch_path}/{license_files[0]}", revision, dest) |
Contributor
There was a problem hiding this comment.
Copy every detected license file, not just the first one.
Line 152 exports only license_files[0]. Repositories that ship LICENSE plus NOTICE/COPYING will lose the extra notice whenever src: filtering is used.
Suggested fix
if license_files:
dest = (
local_path if os.path.isdir(local_path) else os.path.dirname(local_path)
)
- SvnRepo.export(f"{root_branch_path}/{license_files[0]}", revision, dest)
+ for license_file in license_files:
+ SvnRepo.export(f"{root_branch_path}/{license_file}", revision, dest)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/svnsubproject.py` around lines 143 - 152, The
_copy_license_files method only exports the first detected file
(license_files[0]) so additional files like NOTICE or COPYING are skipped;
update _copy_license_files to iterate over all entries returned by
SvnFetcher._license_files(root_branch_path) and call SvnRepo.export for each
file (use the same root_branch_path, revision and computed dest), ensuring dest
is computed once from local_path and used for every export.
spoorcc
force-pushed
the
claude/codescene-refactoring-review-31VM1
branch
from
c24d2fe to
317c6f0
Compare
spoorcc
force-pushed
the
claude/codescene-refactoring-review-31VM1
branch
from
317c6f0 to
20f0eaa
Compare
spoorcc
force-pushed
the
claude/codescene-refactoring-review-31VM1
branch
from
20f0eaa to
d31ab5e
Compare
SubProject was an ABC mixing VCS-specific capabilities with domain orchestration. Archive fetching has no concept of branches/tags/revisions, so forcing it to implement VCS-shaped stubs was a design smell. New domain model: - Fetcher (Protocol): minimal common contract (fetch, freeze, wanted_version, latest_available_version, list_tool_info) - VcsFetcher(Fetcher, Protocol): VCS additions (branches, tags, revisions, browse_tree, patch_type) — git and svn only - AbstractVcsFetcher(ABC): shared latest_available_version + freeze logic, eliminating duplication between GitFetcher and SvnFetcher - ArchiveFetcher: implements Fetcher only — no VCS methods - SubProject: concrete domain aggregate composed with a Fetcher; single dispatch point via as_vcs() -> VcsFetcher | None All cyclomatic complexity kept ≤ 6. Pylint 10.00/10. All 569 unit tests pass. Feature tests pass for git; SVN failures are environmental (no svn). https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
- __init__.py: include remote URL in 'vcs type unsupported' RuntimeError so developers can identify which manifest entry triggered the failure - gitsubproject.py: replace @lru_cache + type: ignore with explicit per-instance lazy cache (_default_branch field), eliminating the shared-across-instances cache issue and the type suppression - pick.py: remove redundant 'n' parameter from _render_pick_lines; compute len(items) inside the function, dropping the too-many-arguments suppression comment - add.py: extract remote.set_remote() call out of _build_entry so the function stays within the 6-argument limit; apply it at each call site, dropping the too-many-arguments suppression comment - test_subproject.py: tighten Version | None annotations on MockVcsFetcher __init__ and _make_subproject; replace six nested with-patch managers with a single contextlib.ExitStack for readability https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
- Add raise NotImplementedError to Protocol methods with non-None return types so pyright accepts the bodies without triggering pylint W2301 - Annotate NAME: str on all concrete fetchers and MockVcsFetcher to satisfy pyright's Protocol invariance check - Rename SvnFetcher.fetch _name -> name (Protocol parameter match); emit a debug log to silence the unused-argument warning - Extract _resolve_entry_version helper from _non_interactive_entry in add.py to reduce cyclomatic complexity from C(11) to B(8) - Fix mypy errors: explicit tuple unpack in license.py, str() casts on Prompt.ask() calls in prompt.py and add.py - Use TYPE_CHECKING stub for LogRender base to avoid "subclass of Any" mypy error without requiring an inline suppression comment https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
add.py: _resolve_entry_version now short-circuits for archive-backed subprojects (as_vcs() is None), returning Version(revision=raw_version) instead of falling through to a VCS branch/tag lookup that always produces an empty branch fallback for archives. log.py: replace _LogRenderBase/_NoExpandLogRender class pair (each requiring a pylint too-few-public-methods suppression) with a single factory function _make_non_expanding_log_render; the only remaining suppression is the pre-existing protected-access on handler._log_render. svnsubproject.py: override latest_available_version so SVN revision-only pins (revision set, no branch, no tag) return the pinned revision with the default branch context; previously the base implementation called latest_revision_on_branch(trunk), causing dfetch check to report pinned SVN revisions as out of date. tests/test_subproject.py: MockVcsFetcher.browse_tree now returns nullcontext(lambda path="": []) so any consumer calling with subproject.as_vcs().browse_tree(...) as ls: gets a valid context manager instead of None. https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
…toring Replace the old SubProject inheritance model (ArchiveSubProject/GitSubproject/ SvnSubproject each implementing abstract SubProject) with the new composition design: SubProject composes with a Fetcher; Fetcher and VcsFetcher as protocols; AbstractVcsFetcher as shared ABC; GitFetcher and SvnFetcher implementing VcsFetcher; ArchiveFetcher implementing Fetcher only. Update AGENTS.md to describe the concrete SubProject aggregate and direct developers to implement Fetcher/VcsFetcher protocols when adding new backends. https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
Bundles source, ignore, and eol_hint into a single FetchContext parameter so fetch() stays within pylint's max-args=6 limit even after EOL support was added. All three concrete fetchers (git, svn, archive), the mock in the unit tests, and SubProject.update() are updated consistently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
spoorcc
force-pushed
the
claude/codescene-refactoring-review-31VM1
branch
from
d31ab5e to
bece1d9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
dfetch/commands/add.py (1)
275-291:⚠️ Potential issue | 🟠 Major | ⚡ Quick winArchive interactive version handling still falls back to VCS branch semantics.
_resolve_entry_version()fixes archive handling, but_interactive_flowstill bypasses it and resolvesoverrides.versionvia branch/tag choices +_resolve_raw_version(...). For archive-backed subprojects, non-SHA inputs still becomebranchinstead ofrevision.Suggested fix
- if overrides.version is not None: - choices: list[Version] = [ - *[ - Version(branch=b) - for b in prioritise_default(branches, ctx.default_branch) - ], - *[Version(tag=t) for t in sort_tags_newest_first(tags)], - ] - version = _resolve_raw_version(overrides.version, choices) or Version( - branch=ctx.default_branch - ) + if overrides.version is not None: + version = _resolve_entry_version(ctx, overrides.version) else: version = _ask_version(ctx.default_branch, branches, tags)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dfetch/commands/add.py` around lines 275 - 291, The `_interactive_flow` function still resolves `overrides.version` using branch and tag choices with `_resolve_raw_version()`, which applies VCS semantics to archive-backed subprojects where they should not apply. In `_interactive_flow`, apply the same logic as `_resolve_entry_version()`: check if the subproject is archive-backed by testing `ctx.subproject.as_vcs() is None`, and if true, preserve the version as a revision identifier instead of attempting to resolve it against branch and tag choices. This ensures archive-backed subprojects consistently use revision semantics rather than incorrectly falling back to branch semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dfetch/log.py`:
- Around line 69-73: The inline pylint suppression on the assignment to
handler._log_render violates the coding guideline against inline lint
suppressions. Create a dedicated RichHandler subclass that internally handles
setting the _log_render attribute with the result of
_make_non_expanding_log_render, then instantiate and use this custom subclass
instead of directly assigning to the handler's private attribute at the call
site. This encapsulates the private API access within the subclass definition,
eliminating the need for the inline pylint suppression while maintaining the
functionality.
In `@dfetch/project/fetcher.py`:
- Around line 148-151: The _resolve_branch method uses a single space as a
sentinel value to indicate an empty branch selection, but this convention lacks
documentation and could confuse maintainers. Either add an inline comment above
or within the wanted_branch == " " condition to explain that a single space is a
special sentinel value meaning "no branch specified", or define a module-level
constant for this sentinel value and reference it in the condition to make the
intent explicit and maintainable.
---
Duplicate comments:
In `@dfetch/commands/add.py`:
- Around line 275-291: The `_interactive_flow` function still resolves
`overrides.version` using branch and tag choices with `_resolve_raw_version()`,
which applies VCS semantics to archive-backed subprojects where they should not
apply. In `_interactive_flow`, apply the same logic as
`_resolve_entry_version()`: check if the subproject is archive-backed by testing
`ctx.subproject.as_vcs() is None`, and if true, preserve the version as a
revision identifier instead of attempting to resolve it against branch and tag
choices. This ensures archive-backed subprojects consistently use revision
semantics rather than incorrectly falling back to branch semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf1afcc2-e12d-496f-b4ab-4d3b465adb08
📒 Files selected for processing (23)
AGENTS.mddfetch/commands/add.pydfetch/commands/environment.pydfetch/commands/format_patch.pydfetch/log.pydfetch/project/__init__.pydfetch/project/archivesubproject.pydfetch/project/fetcher.pydfetch/project/gitsubproject.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/project/svnsubproject.pydfetch/project/svnsuperproject.pydfetch/terminal/pick.pydfetch/terminal/prompt.pydfetch/util/license.pydoc/static/uml/c3_dfetch_components_project.pumlpyproject.tomltests/test_add.pytests/test_archive.pytests/test_subproject.pytests/test_svn.pytests/test_svnsubproject.py
Comment on lines
+69
to
+73
| no_expand = _make_non_expanding_log_render( | ||
| show_time=False, show_level=False, show_path=False | ||
| ) | ||
| handler._log_render = no_expand # pylint: disable=protected-access | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For rich==15.0.0, is there a public RichHandler extension point to disable log table expansion (without setting _log_render directly)?
💡 Result:
In Rich 15.0.0, there is no public extension point or configuration parameter for the RichHandler class designed to disable log table expansion or the associated column-based wrapping [1][2][3][4]. The RichHandler is explicitly architected to render logs using a table-based layout to maintain aligned, readable output in terminal environments [3][5][4]. Consequently, the internal rendering logic—including the use of a table that enforces fixed-width columns—is considered a core part of its design rather than an optional behavior [3][5]. For users who require control over this rendering (e.g., to disable hard wrapping or column expansion), the library maintainers recommend one of the following approaches: 1. Subclassing and overriding: You can create a custom logging handler by inheriting from logging.Handler or by subclassing RichHandler and overriding the emit method to implement your own rendering logic [3][5]. 2. Custom LogRender: While RichHandler uses an internal LogRender class, it is not intended to be exposed as a public API [3][5]. However, as documented in community discussions, some users have achieved specific layout requirements by defining a custom renderer class and assigning it to the _log_render attribute, though this relies on internal implementation details that may change [3][5]. If you are using the handler for output that is not intended for human consumption in a terminal (e.g., piping logs to a file or another process), it is generally recommended to use a standard logging.StreamHandler or a custom handler that does not perform terminal-specific layout formatting [6][5].
Citations:
- 1: https://rich.readthedocs.io/en/latest/%5Fmodules/rich/logging.html
- 2: https://rich.readthedocs.io/en/latest/logging.html
- 3: How to use rich logger without enforced wrapping? Textualize/rich#344
- 4: https://rich.readthedocs.io/en/latest/reference/logging.html
- 5: [REQUEST] Allow RichHandler to receive custom log rendered class Textualize/rich#438
- 6: [BUG] Console in RichHandler is unable to pass TAB characters through to terminal. Textualize/rich#3672
🏁 Script executed:
# First, let's check if dfetch/log.py exists and get its basic structure
fd -t f "log.py" dfetch/Repository: dfetch-org/dfetch
Length of output: 75
🏁 Script executed:
# Read the relevant sections of dfetch/log.py
cat -n dfetch/log.py | head -80Repository: dfetch-org/dfetch
Length of output: 3123
🏁 Script executed:
# Also check if there's an import statement or usage of _AsciinemaRichHandler later in the file
rg "_AsciinemaRichHandler|_log_render|ASCIINEMA_REC" dfetch/log.py -nRepository: dfetch-org/dfetch
Length of output: 447
🏁 Script executed:
# Check if assigning to self._log_render in __init__ would trigger protected-access warnings
# Also verify no other inline suppressions exist in the file
rg "#\s*(noqa|type:\s*ignore|pylint:\s*disable|pyright:\s*ignore)" dfetch/log.py -nRepository: dfetch-org/dfetch
Length of output: 296
🏁 Script executed:
# Verify the exact line count of the file to ensure line references are accurate
wc -l dfetch/log.pyRepository: dfetch-org/dfetch
Length of output: 79
Remove inline suppression by moving handler customization to a dedicated subclass.
Line 72 contains an inline # pylint: disable=protected-access suppression, which violates the coding guideline for dfetch/**/*.py (no inline lint suppressions except module-level headers in lines 1–5). The direct assignment to handler._log_render couples this code to Rich's private API at the call site.
Create a dedicated RichHandler subclass to encapsulate the customization and eliminate the inline suppression:
Suggested refactor
+class _AsciinemaRichHandler(RichHandler):
+ """RichHandler variant with non-expanding log table rendering."""
+
+ def __init__(self, **kwargs: Any) -> None:
+ super().__init__(**kwargs)
+ self._log_render = _make_non_expanding_log_render(
+ show_time=False, show_level=False, show_path=False
+ )
+
def configure_root_logger(console: Console | None = None) -> None:
"""Configure the root logger with RichHandler using the provided Console."""
console = console or make_console()
+ handler_cls: type[RichHandler] = (
+ _AsciinemaRichHandler if os.getenv("ASCIINEMA_REC") else RichHandler
+ )
- handler = RichHandler(
+ handler = handler_cls(
console=console,
show_time=False,
show_path=False,
show_level=False,
markup=True,
rich_tracebacks=True,
highlighter=NullHighlighter(),
)
-
- if os.getenv("ASCIINEMA_REC"):
- # Rich's LogRender uses expand=True on its Table.grid, which pads every
- # log message with trailing spaces to fill the full terminal width. When
- # asciinema records the output the padded line fills the terminal exactly,
- # causing the subsequent newline to produce a blank line in the cast
- # player. Wrapping _log_render so it returns a non-expanding table
- # removes the trailing spaces and avoids the spurious blank lines.
- no_expand = _make_non_expanding_log_render(
- show_time=False, show_level=False, show_path=False
- )
- handler._log_render = no_expand # pylint: disable=protected-accessRich 15.0.0 provides no public API to disable log table expansion; the library maintainers recommend subclassing RichHandler for customization. This refactor aligns with that guidance while removing the guideline violation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/log.py` around lines 69 - 73, The inline pylint suppression on the
assignment to handler._log_render violates the coding guideline against inline
lint suppressions. Create a dedicated RichHandler subclass that internally
handles setting the _log_render attribute with the result of
_make_non_expanding_log_render, then instantiate and use this custom subclass
instead of directly assigning to the handler's private attribute at the call
site. This encapsulates the private API access within the subclass definition,
eliminating the need for the inline pylint suppression while maintaining the
functionality.
Source: Coding guidelines
Comment on lines
+148
to
+151
| def _resolve_branch(self, wanted_branch: str) -> str: | ||
| if wanted_branch == " ": | ||
| return "" | ||
| return wanted_branch or self.get_default_branch() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider documenting the single-space branch sentinel value.
The check for wanted_branch == " " (single space) to return an empty string is a sentinel pattern that could be confusing to maintainers. Consider adding a brief comment or using a named constant to clarify this convention.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/project/fetcher.py` around lines 148 - 151, The _resolve_branch method
uses a single space as a sentinel value to indicate an empty branch selection,
but this convention lacks documentation and could confuse maintainers. Either
add an inline comment above or within the wanted_branch == " " condition to
explain that a single space is a special sentinel value meaning "no branch
specified", or define a module-level constant for this sentinel value and
reference it in the condition to make the intent explicit and maintainable.
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
2 participants
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.
SubProject was an ABC mixing VCS-specific capabilities with domain
orchestration. Archive fetching has no concept of branches/tags/revisions,
so forcing it to implement VCS-shaped stubs was a design smell.
New domain model:
latest_available_version, list_tool_info)
browse_tree, patch_type) — git and svn only
eliminating duplication between GitFetcher and SvnFetcher
dispatch point via as_vcs() -> VcsFetcher | None
All cyclomatic complexity kept ≤ 6. Pylint 10.00/10. All 569 unit tests
pass. Feature tests pass for git; SVN failures are environmental (no svn).
https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
Summary by CodeRabbit