diff --git a/AGENTS.md b/AGENTS.md index 1cd58c40..12c961e0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -194,6 +194,41 @@ def test_sync( - For typing, use `import typing as t` and access via namespace: `t.NamedTuple`, etc. - Use `from __future__ import annotations` at the top of all Python files +### Naming Conventions + +Follow Python community conventions (Django, pytest, Sphinx patterns): + +**Method naming:** +- Use `get_*` prefix for methods that perform I/O or subprocess calls (e.g., `get_remotes()`, `get_revision()`) +- Use `is_*` prefix for boolean checks (e.g., `is_valid()`) +- Use `has_*` prefix for existence checks (e.g., `has_remote()`) + +**Parameter naming:** +- Use descriptive names instead of underscore-prefixed built-in shadows +- BAD: `_all`, `_type`, `_list` (cryptic, non-standard) +- GOOD: `all_remotes`, `include_all`, `file_type`, `path_list` (self-documenting) + +**Examples:** +```python +# BAD - cryptic underscore prefix +def fetch(_all: bool = False): ... +def rev_list(_all: bool = False): ... + +# GOOD - descriptive parameter names +def fetch(all_remotes: bool = False): ... +def rev_list(include_all: bool = False): ... + +# BAD - inconsistent getter naming +def remotes(): ... # No prefix +def get_revision(): ... # Has prefix + +# GOOD - consistent getter naming for subprocess calls +def get_remotes(): ... +def get_revision(): ... +``` + +**Rationale:** Major Python projects (Django, pytest, Sphinx) don't use `_all` style prefixes. They either use the built-in name directly as a keyword-only argument, or use descriptive alternatives. Underscore prefixes are reserved for private/internal parameters only. + ### Docstrings Follow NumPy docstring style for all functions and methods: @@ -468,6 +503,55 @@ Bad: $ pipx install --suffix=@next --pip-args '\--pre' --force 'libvcs' ``` +### CHANGES and MIGRATION Files + +Maintain `CHANGES` (changelog) and `MIGRATION` (upgrade guide) for all user-facing changes. + +**File structure:** +- `CHANGES`: Organized by version with sections in this order of precedence: + 1. `### Breaking changes` - API changes that require user action + 2. `### New features` - New functionality + 3. `### Bug fixes` - Corrections to existing behavior + 4. `### Documentation` - Doc-only changes + 5. `### Development` or `### Internal` - Tooling, CI, refactoring +- `MIGRATION`: Detailed migration instructions with before/after examples + +**Maintenance-only releases:** +For releases with no user-facing changes (only internal/development work), use: +```markdown +_Maintenance only, no bug fixes, or new features_ +``` + +**PR references - where to put them:** +- **DO**: Put PR number in section headers or at end of bullet items in the files +- **DON'T**: Put PR number in commit message titles (causes linkback notification noise in the PR) + +**For larger changes with dedicated sections:** +```markdown +#### API Naming Consistency (#507) + +Renamed parameters and methods... +``` + +**For smaller changes in a list:** +```markdown +### Bug fixes + +- Fix argument expansion in `rev_list` (#455) +- Remove unused command: `Svn.mergelist` (#450) +``` + +**Commit messages should NOT include PR numbers:** +```bash +# GOOD - no PR in commit message +CHANGES(docs): Document breaking API changes for 0.39.x + +# BAD - PR in commit message creates noise +CHANGES(docs): Document breaking API changes for 0.39.x (#507) +``` + +The PR reference in the file content creates a clean linkback when the PR merges, while keeping commit messages focused and avoiding duplicate notifications. + ## Debugging Tips When stuck in debugging loops: diff --git a/CHANGES b/CHANGES index 2aa6ea5f..047185c2 100644 --- a/CHANGES +++ b/CHANGES @@ -20,6 +20,49 @@ $ uv add libvcs --prerelease allow _Notes on the upcoming release will go here._ +### Breaking changes + +#### API Naming Consistency (#507) + +Renamed parameters and methods to follow Python community conventions (Django, pytest, Sphinx patterns). + +**cmd/git.py: Renamed `_all` parameters to descriptive names:** + +| Method | Old Parameter | New Parameter | +|--------|---------------|---------------| +| `clone()` | `_all` | `all_branches` | +| `fetch()` | `_all` | `all_remotes` | +| `pull()` | `_all` | `all_remotes` | +| `help()` | `_all` | `show_all` | +| `rev_list()` | `_all` | `include_all_refs` | +| `remote.get_url()` | `_all` | `all_urls` | +| `stash.save()` | `_all` | `include_ignored` | +| `stash.push()` | `_all` | `include_ignored` | +| `branches.ls()` | `_all` | `all_branches` | + +**sync/git.py: Renamed getter methods with `get_` prefix:** + +| Old Method | New Method | +|------------|------------| +| `remotes()` | `get_remotes()` | +| `remote(name)` | `get_remote(name)` | + +**Migration example:** + +```python +# Before +git.fetch(_all=True) +git.stash.save(_all=True) +repo.remotes() +repo.remote("origin") + +# After +git.fetch(all_remotes=True) +git.stash.save(include_ignored=True) +repo.get_remotes() +repo.get_remote("origin") +``` + ## libvcs 0.40.0 (2026-04-25) ### What's new diff --git a/MIGRATION b/MIGRATION index 219c8472..e1bff671 100644 --- a/MIGRATION +++ b/MIGRATION @@ -24,6 +24,63 @@ _Notes on the upcoming release will be added here_ +### API Naming Consistency (#507) + +Renamed parameters and methods to follow Python community conventions (Django, pytest, Sphinx patterns). + +#### cmd/git.py: Renamed `_all` parameters + +The `_all` parameter has been renamed to descriptive names across multiple methods: + +| Method | Old | New | +|--------|-----|-----| +| `clone()` | `_all` | `all_branches` | +| `fetch()` | `_all` | `all_remotes` | +| `pull()` | `_all` | `all_remotes` | +| `help()` | `_all` | `show_all` | +| `rev_list()` | `_all` | `include_all_refs` | +| `remote.get_url()` | `_all` | `all_urls` | +| `stash.save()` | `_all` | `include_ignored` | +| `stash.push()` | `_all` | `include_ignored` | +| `branches.ls()` | `_all` | `all_branches` | + +Before: + +```python +git.fetch(_all=True) +git.stash.save(_all=True) +``` + +After: + +```python +git.fetch(all_remotes=True) +git.stash.save(include_ignored=True) +``` + +#### sync/git.py: Renamed getter methods + +Added `get_` prefix for consistency with other getter methods: + +| Old | New | +|-----|-----| +| `remotes()` | `get_remotes()` | +| `remote(name)` | `get_remote(name)` | + +Before: + +```python +repo.remotes() +repo.remote("origin") +``` + +After: + +```python +repo.get_remotes() +repo.get_remote("origin") +``` + ### pytest fixtures: `git_local_clone` renamed to `example_git_repo` (#468) - pytest: `git_local_clone` renamed to `example_git_repo` diff --git a/src/libvcs/cmd/git.py b/src/libvcs/cmd/git.py index 1bdb0890..bd6345cb 100644 --- a/src/libvcs/cmd/git.py +++ b/src/libvcs/cmd/git.py @@ -309,7 +309,7 @@ def clone( jobs: str | None = None, force: bool | None = None, local: bool | None = None, - _all: bool | None = None, + all_branches: bool | None = None, no_hardlinks: bool | None = None, hardlinks: bool | None = None, shared: bool | None = None, @@ -441,7 +441,7 @@ def fetch( recurse_submodules: bool | t.Literal["yes", "on-demand", "no"] | None = None, recurse_submodules_default: bool | t.Literal["yes", "on-demand"] | None = None, submodule_prefix: StrOrBytesPath | None = None, - _all: bool | None = None, + all_remotes: bool | None = None, force: bool | None = None, keep: bool | None = None, multiple: bool | None = None, @@ -527,7 +527,7 @@ def fetch( local_flags.append("--progress") if verbose: local_flags.append("--verbose") - if _all: + if all_remotes: local_flags.append("--all") if atomic: local_flags.append("--atomic") @@ -851,7 +851,7 @@ def pull( # fetch: bool | None = None, no_fetch: bool | None = None, - _all: bool | None = None, + all_remotes: bool | None = None, force: bool | None = None, keep: bool | None = None, multiple: bool | None = None, @@ -1018,7 +1018,7 @@ def pull( local_flags.append("--progress") if verbose: local_flags.append("--verbose") - if _all: + if all_remotes: local_flags.append("--all") if atomic: local_flags.append("--atomic") @@ -1331,7 +1331,7 @@ def init( def help( self, *, - _all: bool | None = None, + show_all: bool | None = None, verbose: bool | None = None, no_external_commands: bool | None = None, no_aliases: bool | None = None, @@ -1348,17 +1348,17 @@ def help( Parameters ---------- - _all : bool + show_all : bool Prints everything. no_external_commands : bool - For use with ``all``, excludes external commands. + For use with ``show_all``, excludes external commands. no_aliases : bool - For use with ``all``, excludes aliases. + For use with ``show_all``, excludes aliases. verbose : bool - For us with ``all``, on by default. + For us with ``show_all``, on by default. config : bool List all config vars. @@ -1382,7 +1382,7 @@ def help( >>> git.help() "usage: git [...--version] [...--help] [-C ]..." - >>> git.help(_all=True) + >>> git.help(show_all=True) "See 'git help ' to read about a specific subcommand..." >>> git.help(info=True) @@ -1395,7 +1395,7 @@ def help( if verbose is True: local_flags.append("--verbose") - if _all is True: + if show_all is True: local_flags.append("--all") if no_external_commands is True: local_flags.append("--no-external-commands") @@ -2087,7 +2087,7 @@ def rev_list( first_parent: bool | None = None, exclude_first_parent_only: bool | None = None, _not: bool | None = None, - _all: bool | None = None, + include_all_refs: bool | None = None, branches: str | bool | None = None, tags: str | bool | None = None, remotes: str | bool | None = None, @@ -2145,7 +2145,9 @@ def rev_list( >>> git.rev_list(commit="HEAD", path=".", max_count=1, header=True) '...' - >>> git.rev_list(commit="origin..HEAD", max_count=1, _all=True, header=True) + >>> git.rev_list( + ... commit="origin..HEAD", max_count=1, include_all_refs=True, header=True + ... ) '...' >>> git.rev_list(commit="origin..HEAD", max_count=1, header=True) @@ -2202,7 +2204,7 @@ def rev_list( for flag, shell_flag in [ # Limiting output - (_all, "--all"), + (include_all_refs, "--all"), (author, "--author"), (committer, "--committer"), (grep, "--grep"), @@ -2221,7 +2223,7 @@ def rev_list( (first_parent, "--first-parent"), (exclude_first_parent_only, "--exclude-first-parent-only"), (_not, "--not"), - (_all, "--all"), + (include_all_refs, "--all"), (exclude, "--exclude"), (reflog, "--reflog"), (alternative_refs, "--alternative-refs"), @@ -3902,7 +3904,7 @@ def get_url( self, *, push: bool | None = None, - _all: bool | None = None, + all_urls: bool | None = None, # Pass-through to run() log_in_real_time: bool = False, check_returncode: bool | None = None, @@ -3927,7 +3929,7 @@ def get_url( >>> GitRemoteCmd( ... path=example_git_repo.path, ... remote_name='origin' - ... ).get_url(_all=True) + ... ).get_url(all_urls=True) 'file:///...' """ local_flags: list[str] = [] @@ -3935,7 +3937,7 @@ def get_url( if push: local_flags.append("--push") - if _all: + if all_urls: local_flags.append("--all") return self.run( @@ -4971,7 +4973,7 @@ def save( keep_index: int | None = None, patch: bool | None = None, include_untracked: bool | None = None, - _all: bool | None = None, + include_ignored: bool | None = None, quiet: bool | None = None, # Pass-through to run() log_in_real_time: bool = False, @@ -4991,7 +4993,7 @@ def save( local_flags: list[str] = [] stash_flags: list[str] = [] - if _all is True: + if include_ignored is True: local_flags.append("--all") if staged is True: local_flags.append("--staged") @@ -5100,7 +5102,7 @@ def push( staged: bool | None = None, keep_index: bool | None = None, include_untracked: bool | None = None, - _all: bool | None = None, + include_ignored: bool | None = None, quiet: bool | None = None, # Pass-through to run() log_in_real_time: bool = False, @@ -5124,7 +5126,7 @@ def push( Keep index intact (-k) include_untracked : Include untracked files (-u) - _all : + include_ignored : Include ignored files (-a) quiet : Suppress output (-q) @@ -5150,7 +5152,7 @@ def push( local_flags.append("-k") if include_untracked is True: local_flags.append("-u") - if _all is True: + if include_ignored is True: local_flags.append("-a") if quiet is True: local_flags.append("-q") @@ -5762,7 +5764,7 @@ def _ls( def ls( self, *, - _all: bool = False, + all_branches: bool = False, remotes: bool = False, merged: str | None = None, no_merged: str | None = None, @@ -5774,7 +5776,7 @@ def ls( Parameters ---------- - _all : + all_branches : List all branches (local + remote). Maps to --all. remotes : List remote branches only. Maps to --remotes. @@ -5796,7 +5798,7 @@ def ls( True """ local_flags: list[str] = [] - if _all: + if all_branches: local_flags.append("--all") if remotes: local_flags.append("--remotes") diff --git a/src/libvcs/sync/git.py b/src/libvcs/sync/git.py index 42d3f98f..5bc2db3d 100644 --- a/src/libvcs/sync/git.py +++ b/src/libvcs/sync/git.py @@ -324,7 +324,7 @@ def set_remotes(self, overwrite: bool = False) -> None: remotes = self._remotes if isinstance(remotes, dict): for remote_name, git_remote_repo in remotes.items(): - existing_remote = self.remote(remote_name) + existing_remote = self.get_remote(remote_name) if isinstance(git_remote_repo, GitRemote): if ( not existing_remote @@ -336,7 +336,7 @@ def set_remotes(self, overwrite: bool = False) -> None: overwrite=overwrite, ) # refresh if we're setting it, so push can be checked - existing_remote = self.remote(remote_name) + existing_remote = self.get_remote(remote_name) if git_remote_repo.push_url and ( not existing_remote or existing_remote.push_url != git_remote_repo.push_url @@ -639,7 +639,7 @@ def update_repo( result.add_error("submodule-update", str(e), exception=e) return result - def remotes(self) -> GitSyncRemoteDict: + def get_remotes(self) -> GitSyncRemoteDict: """Return remotes like git remote -v. Parameters @@ -657,12 +657,12 @@ def remotes(self) -> GitSyncRemoteDict: for r in ret: # FIXME: Cast to the GitRemote that sync uses, for now - remote = self.remote(r.remote_name) + remote = self.get_remote(r.remote_name) if remote is not None: remotes[r.remote_name] = remote return remotes - def remote(self, name: str, **kwargs: t.Any) -> GitRemote | None: + def get_remote(self, name: str, **kwargs: t.Any) -> GitRemote | None: """Get the fetch and push URL for a specified remote name. Parameters @@ -738,7 +738,7 @@ def set_remote( else: self.cmd.remotes.add(name=name, url=url, check_returncode=True) - remote = self.remote(name=name) + remote = self.get_remote(name=name) if remote is None: raise GitRemoteSetError(remote_name=name) return remote diff --git a/tests/cmd/test_git.py b/tests/cmd/test_git.py index 9434ed7d..19663c61 100644 --- a/tests/cmd/test_git.py +++ b/tests/cmd/test_git.py @@ -630,7 +630,7 @@ def test_branch_ls_filters(git_repo: GitSync) -> None: assert any(b.branch_name == "master" for b in branches) # Test with --all (includes remote-tracking branches) - all_branches = git_repo.cmd.branches.ls(_all=True) + all_branches = git_repo.cmd.branches.ls(all_branches=True) assert len(all_branches) >= len(branches) # Test with --merged (branches merged into HEAD) @@ -2688,19 +2688,19 @@ def test_submodule_entry_absorbgitdirs( def test_rev_list_all_parameter(git_repo: GitSync) -> None: - """Test that _all parameter controls --all flag in rev-list.""" + """Test that include_all_refs parameter controls --all flag in rev-list.""" # Create a branch with an extra commit not reachable from master git_repo.cmd.run(["checkout", "-b", "other-branch"]) git_repo.cmd.run(["commit", "--allow-empty", "-m", "other-branch-commit"]) git_repo.cmd.run(["checkout", "master"]) - # Without _all: only commits reachable from HEAD (master) + # Without include_all_refs: only commits reachable from HEAD (master) result_no_all = git_repo.cmd.rev_list(commit="HEAD") count_no_all = len(result_no_all.strip().split("\n")) - # With _all=True: includes commits from all branches - result_with_all = git_repo.cmd.rev_list(commit="HEAD", _all=True) + # With include_all_refs=True: includes commits from all branches + result_with_all = git_repo.cmd.rev_list(commit="HEAD", include_all_refs=True) count_with_all = len(result_with_all.strip().split("\n")) - # _all=True should return strictly more commits (the other-branch commit) + # With include_all_refs=True we should get strictly more commits. assert count_with_all > count_no_all diff --git a/tests/sync/test_git.py b/tests/sync/test_git.py index ecb253ce..9a5dae07 100644 --- a/tests/sync/test_git.py +++ b/tests/sync/test_git.py @@ -448,7 +448,7 @@ def test_remotes( expected = lazy_remote_expected(**locals()) assert len(expected.keys()) > 0 for expected_remote_name, expected_remote_dict in expected.items(): - remote = git_repo.remote(expected_remote_name) + remote = git_repo.get_remote(expected_remote_name) assert remote is not None if remote is not None: @@ -574,7 +574,7 @@ def test_remotes_update_repo( expected = lazy_remote_expected(**locals()) assert len(expected.keys()) > 0 for expected_remote_name, expected_remote in expected.items(): - assert expected_remote == git_repo.remote(expected_remote_name) + assert expected_remote == git_repo.get_remote(expected_remote_name) def test_git_get_url_and_rev_from_pip_url() -> None: @@ -641,7 +641,7 @@ def test_remotes_preserves_git_ssh( git_repo.set_remote(name=remote_name, url=remote_url) assert GitRemote(remote_name, remote_url, remote_url) in list( - git_repo.remotes().values(), + git_repo.get_remotes().values(), ) @@ -685,8 +685,8 @@ def test_private_ssh_format( def test_git_sync_remotes(git_repo: GitSync) -> None: - """Test GitSync.remotes().""" - remotes = git_repo.remotes() + """Test GitSync.get_remotes().""" + remotes = git_repo.get_remotes() assert "origin" in remotes assert git_repo.cmd.remotes.show() == "origin" @@ -694,7 +694,7 @@ def test_git_sync_remotes(git_repo: GitSync) -> None: assert git_origin is not None assert "origin" in git_origin.show() assert "origin" in git_origin.show(no_query_remotes=True) - assert git_repo.remotes()["origin"].name == "origin" + assert git_repo.get_remotes()["origin"].name == "origin" @pytest.mark.parametrize( @@ -710,15 +710,15 @@ def test_set_remote(git_repo: GitSync, repo_name: str, new_repo_url: str) -> Non assert "file:///" in mynewremote.fetch_url, "set_remote returns remote" assert isinstance( - git_repo.remote(name=repo_name), + git_repo.get_remote(name=repo_name), GitRemote, - ), "remote() returns GitRemote" - remote = git_repo.remote(name=repo_name) + ), "get_remote() returns GitRemote" + remote = git_repo.get_remote(name=repo_name) assert remote is not None, "Remote should exist" if remote is not None: assert "file:///" in remote.fetch_url, "new value set" - assert "myrepo" in git_repo.remotes(), ".remotes() returns new remote" + assert "myrepo" in git_repo.get_remotes(), ".get_remotes() returns new remote" with pytest.raises( exc.CommandError, @@ -728,7 +728,7 @@ def test_set_remote(git_repo: GitSync, repo_name: str, new_repo_url: str) -> Non mynewremote = git_repo.set_remote(name="myrepo", url=new_repo_url, overwrite=True) - remote = git_repo.remote(name="myrepo") + remote = git_repo.get_remote(name="myrepo") assert remote is not None if remote is not None: assert new_repo_url in remote.fetch_url, ( @@ -1705,19 +1705,19 @@ def test_remote_is_fast_for_repos_with_many_refs( ) started = time.monotonic() - remote = git_repo.remote("origin") + remote = git_repo.get_remote("origin") elapsed = time.monotonic() - started assert remote is not None assert remote.fetch_url, "fetch URL must be populated" - assert elapsed < 5.0, f"remote() too slow: {elapsed:.2f}s with 500 fake refs" + assert elapsed < 5.0, f"get_remote() too slow: {elapsed:.2f}s with 500 fake refs" def test_remote_swallows_libvcs_exception( git_repo: GitSync, mocker: MockerFixture, ) -> None: - """``GitSync.remote`` returns ``None`` when ``git remote -v`` fails. + """``GitSync.get_remote`` returns ``None`` when ``git remote -v`` fails. The pre-rewrite implementation wrapped the underlying subprocess call in ``try / except LibVCSException`` so a corrupt config or a locked @@ -1735,4 +1735,4 @@ def test_remote_swallows_libvcs_exception( ), ) - assert git_repo.remote("origin") is None + assert git_repo.get_remote("origin") is None