Skip to content

add pagination to listBranches and add method listAllBranches #50

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
atharvadeosthale wants to merge 3 commits into
base: main
Choose a base branch
from
Open

add pagination to listBranches and add method listAllBranches #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f561769
add pagination to listBranches and add method listAllBranches
atharvadeosthale Dec 16, 2025
97b0ad3
remove listAllBranches and add tests
atharvadeosthale Dec 16, 2025
6d91945
Merge branch 'main' into branch-pagination
atharvadeosthale Dec 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/VCS/Adapter/Git/GitHub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,11 +464,13 @@ public function getPullRequestFromBranch(string $owner, string $repositoryName,
*
* @param string $owner Owner name of the repository
* @param string $repositoryName Name of the GitHub repository
* @param int $page Page number for pagination
* @param int $perPage Number of results per page
* @return array<string> List of branch names as array
*/
public function listBranches(string $owner, string $repositoryName): array
public function listBranches(string $owner, string $repositoryName, int $page = 1, int $perPage = 30): array
Comment on lines +467 to +471
Copy link

@coderabbitai coderabbitai bot Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify abstract method signature and find all adapter implementations

echo "=== Checking abstract method definition in Adapter.php ==="
rg -n -A 2 "abstract.*function listBranches" --type=php

echo -e "\n=== Finding all implementations of listBranches ==="
rg -n -B 2 -A 5 "public function listBranches" --type=php

echo -e "\n=== Finding all classes that extend Adapter or Git ==="
rg -n "class.*extends.*(Adapter|Git)" --type=php

Repository: utopia-php/vcs

Length of output: 1648

🏁 Script executed:

#!/bin/bash
# Check if there are other VCS adapters beyond GitHub
echo "=== Looking for all Adapter subclasses ==="
rg -n "class.*extends.*Adapter|class.*extends.*Git" --type=php

echo -e "\n=== Checking for all branches method calls in tests or elsewhere ==="
rg -n "listBranches" --type=php | head -20

Repository: utopia-php/vcs

Length of output: 896

Update the abstract method signature to include pagination parameters for consistency.

The listBranches method signature in src/VCS/Adapter.php (line 196) does not include the optional pagination parameters added to the GitHub implementation:

abstract public function listBranches(string $owner, string $repositoryName): array;

While the default values (int $page = 1, int $perPage = 30) in the GitHub implementation maintain backward compatibility for existing callers, the abstract method definition should be updated to match the implementation for architectural consistency and clarity about the interface contract.

🤖 Prompt for AI Agents 
In src/VCS/Adapter.php around line 196, the abstract method listBranches lacks
the optional pagination parameters added in the GitHub implementation; update
the abstract signature to match by adding int $page = 1, int $perPage = 30 to
the method declaration and update its docblock return/type hints accordingly so
the interface contract matches implementations.

Copy link
Author

@atharvadeosthale atharvadeosthale Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worried if some other providers would have different ways / not have pagination at all. We can add it in if the next provider has similar behaviours.

Copy link

@coderabbitai coderabbitai bot Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atharvadeosthale That's a very reasonable approach! Waiting to see how pagination works across multiple providers before standardizing the interface is a sound architectural decision. The default parameters you've added (int $page = 1, int $perPage = 30) already ensure backward compatibility for existing callers, so there's no breaking change risk.

Your strategy of deferring the abstract method update until there's empirical evidence from a second provider implementation follows good design principles and avoids premature abstraction. This keeps the interface flexible for now while the GitHub implementation validates the pagination pattern.

Copy link
Member

@ItzNotABug ItzNotABug Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worried if some other providers would have different ways / not have pagination at all. We can add it in if the next provider has similar behaviours.

If there's no pagination on an X provider, we can simply not use the variable. Its best that the base adapter is the source of truth with needed params. cc @atharvadeosthale

Copy link

@coderabbitai coderabbitai bot Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

{
$url = "/repos/$owner/$repositoryName/branches";
$url = "/repos/$owner/$repositoryName/branches?page=$page&per_page=$perPage";

$response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"]);

Expand Down
18 changes: 18 additions & 0 deletions tests/VCS/Adapter/GitHubTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,4 +426,22 @@ public function testGetLatestCommit(): void
$this->assertSame('https://avatars.githubusercontent.com/u/43381712?v=4', $commitDetails['commitAuthorAvatar']);
$this->assertSame('https://github.com/vermakhushboo', $commitDetails['commitAuthorUrl']);
}

public function testListBranches(): void
{
$owner = 'test-kh';
$repositoryName = 'test2';

// Basic test
$branches = $this->vcsAdapter->listBranches($owner, $repositoryName);
$this->assertIsArray($branches);
$this->assertNotEmpty($branches);
$this->assertContains('main', $branches);
$this->assertContains('test', $branches);

// Test with pagination params
$branches = $this->vcsAdapter->listBranches($owner, $repositoryName, 1, 1);
$this->assertIsArray($branches);
$this->assertCount(1, $branches);
}
}