Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #538 +/- ##
==========================================
+ Coverage 84.24% 84.27% +0.03%
==========================================
Files 29 29
Lines 3795 3809 +14
Branches 756 758 +2
==========================================
+ Hits 3197 3210 +13
+ Misses 378 377 -1
- Partials 220 222 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
why: `Path.replace()` operates on directory entries, not file contents. When the target is a symlink, the symlink itself gets replaced by the temp file — the real target file is left stale. This breaks dotfile setups where `~/.vcspull.yaml` symlinks to a managed location. what: - Resolve target path before creating temp file and renaming - Temp file now created next to the real destination (also fixes latent cross-device rename when symlink spans filesystems)
why: When no `-f` flag is given, config paths returned by `find_home_config_files()` used `.expanduser()` only — the symlink path was passed downstream. The `-f` codepaths already use `.expanduser().resolve()`. This makes discovery consistent. what: - Add `.resolve()` after `.expanduser()` for both YAML and JSON paths
why: No existing tests verified that config writes through symlinks preserve the link. These tests prevent future regressions. what: - Test _atomic_write preserves symlink and updates real target - Test _atomic_write preserves file permissions through symlink - Test save_config end-to-end through symlink - Test find_home_config_files returns resolved path for symlinks
… behavior why: _atomic_write had no doctest coverage. Inline examples show basic usage and the symlink-preservation guarantee introduced in this branch. what: - Add doctest for basic atomic write - Add doctest demonstrating symlink preservation
…add param/return sections why: Docstring did not reflect the behavioral change from adding `.resolve()`, and was missing NumPy-style parameter/return sections. what: - Note that returned paths are resolved through symlinks - Add Parameters and Returns sections per NumPy docstring standard
why: find_home_config_files had no doctest coverage. Inline example shows the empty-return case when no home config exists. what: - Add doctest demonstrating empty return when no home config exists
why: Symlinked config entries in `$HOME` or via `-f/--file` can point to extensionless or differently named targets. Resolving those paths before format detection changes the suffix that downstream code sees, which can make vcspull parse or save with the wrong format or reject supported symlinked dotfile setups outright. what: - Add a shared config-path normalizer that expands paths without resolving symlinks - Use the logical config path for home discovery and explicit config path handling in add, discover, fmt, and import - Keep `_atomic_write()` following the real target so writes still update the destination file while preserving the symlink entry - Add regressions for home config discovery, explicit config path resolution, and end-to-end `vcspull add` behavior through a symlinked home config
why: The first symlink-path fix preserved visible config names, which fixed extensionless dotfile targets but broke alias paths and mismatched symlinks. An explicit alias like `vcspull-config -> .vcspull.yaml` was rejected for having no suffix, and a home link like `.vcspull.yaml -> vcspull.json` could be rewritten with YAML because later format checks only saw the visible path. what: - Add shared config-format detection that prefers a symlink target's supported suffix and otherwise falls back to the visible path suffix - Use that helper for config reads, duplicate-aware YAML loading, save dispatch, ordered-item saves, and import config-path validation - Add regressions for target-format precedence, extensionless alias paths, and end-to-end JSON writes through a symlinked home config
why: Both functions lacked Examples sections despite the project requiring working doctests on all public functions. what: - Add Examples section to save_config_yaml with round-trip read check - Add Examples section to save_config_json with json.loads round-trip check
…ctest why: Function had only a one-line summary with no Parameters, Returns, or Examples section despite the project requiring full NumPy docstrings and working doctests on all public functions. what: - Add description explaining why this differs from save_config_yaml (preserves duplicate top-level keys via ordered pairs) - Add Parameters and Returns sections per NumPy docstring standard - Add Examples section with duplicate-section round-trip doctest
… landed
why: The trigger condition in the TODO ("once the new vcspull add flow
lands") was already satisfied — vcspull add now uses DuplicateAwareConfigReader
for reading. The stale "Revisit once" sentence misled readers into thinking
the integration was still pending.
what:
- Replace the stale "Revisit once the new vcspull add flow lands" sentence
with a note reflecting the current state
- Note that this basic loader remains for simpler read contexts
- Identify Option 1 (shared utility) as the cleanest long-term path
why: Document the symlink-preservation fix and format-detection improvement shipped in this branch. what: - Add Bug fixes entry under v1.59.x unreleased section
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
1 participant
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.
Summary
Resolves #537.
Config files that are symbolic links (e.g.
~/.vcspull.yamlpointing to a dotfilesdirectory) were being silently replaced by regular files on every write, destroying the
symlink. This PR fixes the full write and format-detection path:
_atomic_write(): resolve symlinks before creating the temp file and renaming, so the rename replaces the real target file — the symlink directory entry is preservednormalize_config_file_path(): new helper that expands~and env vars without calling.resolve(), so the logical config name (e.g..vcspull.yaml) is preserved for display and format detectionconfig_format_from_path(): new helper that inspects the symlink target's extension when present, so a.yamlsymlink pointing to a.jsonfile serialises correctly as JSONadd,discover,fmt,import): switch from.expanduser().resolve()tonormalize_config_file_path()so symlinked configs are handled correctly throughoutTest plan
test_atomic_write_through_symlink_preserves_symlink— symlink survives, target updated, no temp file leftoverstest_atomic_write_through_symlink_preserves_permissions— file mode preserved through symlink writetest_save_config_via_symlink_preserves_link— end-to-end throughsave_config()test_find_home_config_files_preserves_symlink_suffix— returned path keeps logical.yaml/.jsonsuffixtest_config_format_from_path_prefers_supported_symlink_target—.yaml→.jsonsymlink returns"json"test_add_repo_uses_home_symlink_config_without_losing_yaml_suffix— add round-trip through extensionless symlink targettest_add_repo_uses_target_json_format_for_home_symlink— add correctly writes JSON when target is.jsontest_resolve_config_file_accepts_extensionless_symlink_alias— explicit-fpath inherits target format