vcspull add - Refactoring and fixes
#480
Merged
Conversation
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #480 +/- ##
==========================================
+ Coverage 78.70% 78.82% +0.12%
==========================================
Files 13 13
Lines 1592 1738 +146
Branches 341 369 +28
==========================================
+ Hits 1253 1370 +117
- Misses 213 232 +19
- Partials 126 136 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tony
force-pushed
the
add-refactor
branch
5 times, most recently
from
f748d83 to
30d03f4
Compare
why: The add_from_fs CLI module was removed, so our logger propagation test should no longer expect that logger. what: - drop the add_from_fs logger name from the propagation assertions - leave the remaining CLI logger checks intact so coverage stays the same
why: ConfigReader still loses duplicate workspace roots while the new formatter keeps them, and we need to track the follow-up work. what: - document the divergence with fmt’s loader in a TODO block - outline options for sharing or replacing the loader once the new add flow lands config/reader(chore[todo]): plan duplicate-aware loader alignment
why: Loading configs with duplicate workspace roots dropped earlier entries, causing data loss when commands rewrote the file. what: - introduce DuplicateAwareConfigReader that records duplicate top-level keys using a SafeLoader hook - expose helpers to return both the parsed config and duplicate occurrences for downstream merging
why: The new loader needs regression tests to ensure duplicates are tracked for YAML and ignored for JSON. what: - add fixtures that capture duplicate YAML sections and confirm both entries are recorded - cover non-duplicate YAML and JSON inputs to prove the loader behaves like the legacy path otherwise
why: fmt already had bespoke duplicate-merging logic; now that we have a shared loader we can drop the custom YAML plumbing. what: - add reusable merge helpers in vcspull.config - switch fmt to DuplicateAwareConfigReader.load_with_duplicates and the shared merge helpers - remove the local SafeLoader subclass and copy helpers
why: Users expect `vcspull add <path>` to detect a repo name/remote and confirm before writing, but the command only supported name + URL. what: - replace the positional arguments with `target` plus optional URL and support `--name` - detect git remotes for path inputs, fall back to local path, and prompt (with `--yes` support) before writing - reuse duplicate-aware loading/merge helpers so path imports don’t drop existing workspace entries
why: The new path-first add flow needs regression tests for dry-run, confirmation, and duplicate scenarios. what: - add parametrized fixtures for auto-confirm, interactive accept/decline, no-remote, and no-merge cases - normalize log output and assert config contents to match the new behavior
why: The changelog should mention the path-based add enhancements delivered in this series. what: - document the duplicate-aware loading, path detection, and confirmation changes under Improvements
why: Some workflows want to inspect duplicate roots without auto-merging; add/discover always merged, risking unintended writes. what: - add a shared `--no-merge` flag that skips automatic merging while still logging warnings for duplicates - thread the optional merge behavior through add_repo and discover_repos using the duplicate-aware loader helpers - keep merge-on as the default so existing behavior stays intact
why: The new `--no-merge` flag must be regression-tested for both add and discover commands. what: - extend the CLI test suites with merge/no-merge parametrized fixtures - normalize log output and verify config contents when duplicates are kept separate
why: Merge/no-merge flows change CLI logs; snapshots need to reflect the new output. what: - record Syrupy snapshots for the updated add path-mode cases - add a discover snapshot covering the no-merge warning messaging
why: Document the new flag so users spot the opt-out quickly. what: - mention the shared --no-merge switch in the improvements section
why: Commands using load_configs were still dropping earlier workspace-root entries, so duplicate paths silently lost repositories outside fmt/add/discover. what: - load configs through DuplicateAwareConfigReader and merge duplicates by default while logging conflicts or skipped merges - keep an opt-out via merge_duplicates=False for callers that want legacy behavior - add regression tests ensuring both paths keep expected repos and emit logs
why: Users reading the changelog need to know list/status/sync now benefit from the duplicate-aware loader introduced for fmt/add/discover. what: - highlight that load_configs merges duplicate workspace roots by default so CLI reads no longer drop repositories - document the merge_duplicates opt-out for workflows that still need the previous "last occurrence wins" semantics
why: Chained string literals are harder to read/update than a single YAML block when crafting duplicate-root fixtures. what: - build the duplicate workspace config with a triple-quoted string via textwrap.dedent for clarity - import textwrap alongside existing logging dependency
why: Several fixtures still built config blobs from chained string literals, which are harder to tweak than a single dedented block. what: - use textwrap.dedent triple-quoted strings for duplicate-root YAML and JSON in test_config_reader - switch the vcspull add duplicate-root fixture to the same style and import textwrap alongside existing helpers
why: The previous changelog entry listed internal implementation details; users need a product-level summary of what happens by default and the flags available. what: - reword the `vcspull add`, `discover`, and loader sections to describe user experience, default behavior, and opt-outs in plain language - keep existing section structure while aligning phrasing with the rest of the release notes
why: The fmt and formatter-test entries still read like implementation notes; we want product-level phrasing consistent with the rest of the changelog. what: - explain that `vcspull fmt` now merges duplicate workspace sections by default and point to `--no-merge` for inspection-only runs - describe Syrupy snapshot coverage as a guardrail for future fmt changes
why: User-facing docs and README still describe the pre-merge behavior for add, discover, and fmt; guidance should mention the new defaults without drowning out other tips. what: - note that fmt merges duplicate workspace roots unless --no-merge is supplied - explain path-based adds, confirmation flags, and duplicate handling in docs/cli/add.md and the README bullet list - mention discover's default merge behavior with a concise pointer to --no-merge
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.
Changes
Improvements
Duplicate-aware config loading
load_configsnow reuses the duplicate-aware reader sovcspull list,status, andsynckeep every repository even when workspace roots repeat.Safer add/discover workflows
vcspull addgained a path-first flow that inspects existing checkouts,detects the
originremote, shortens paths to~/…, and confirms beforewriting by default. Use
--yesfor unattended scripts.vcspull discovermirrors the merge-by-default behavior while offering--no-mergefor manual review.Formatter defaults
vcspull fmtconsolidates duplicate workspace sections automatically andsurfaces conflicts;
--no-mergeleaves duplicates in place for inspection.Documentation
CLI guides and README
path-based adds, confirmation flags, and formatter options.
cli/importpage now thatvcspull addfully replaces theold command.
Development
Test coverage
JSON/YAML snapshots for golden coverage.
Test plan