Skip to content

Conversation

@Nyholm
Copy link
Member

@Nyholm Nyholm commented May 29, 2025

Q A
Bug fix? no
New feature? yes
Docs? no
Issues #5
License MIT

Just following the protocol.

Note that there is no real benefit with pagination for our standard Collection implementation

@Nyholm Nyholm requested a review from chr-hertel as a code owner May 29, 2025 18:25
@Nyholm Nyholm mentioned this pull request May 29, 2025
9 tasks
@chr-hertel
Copy link
Member

Note that there is no real benefit with pagination for our standard Collection implementation

you mean because everything is still in the collection anyways?

@Nyholm
Copy link
Member Author

Nyholm commented May 29, 2025

you mean because everything is still in the collection anyways?

Yes, But another implementation might have 1000 items and read them from disk or database. They could benefit.

public function __construct(
private readonly CollectionInterface $toolCollection,
private readonly CollectionInterface $collection,
private readonly int $pageSize = 20,
Copy link
Member

Choose a reason for hiding this comment

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

that introduces paging by default without the possibility to opt out, right?
well, one could set PHP_INT_MAX here, but what about allowing null to disable it?

anyhow, totally minor thing - valid to merge anyways

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Let's remember that we should fix this.

(I created an issue)

Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

Looks good - thanks! :)

@Nyholm Nyholm force-pushed the feat-pagination branch from dc52547 to 1fc5e9c Compare May 29, 2025 19:45
@Nyholm Nyholm merged commit 886307f into main May 29, 2025
24 checks passed
@chr-hertel chr-hertel deleted the feat-pagination branch May 29, 2025 19:47
* file that was distributed with this source code.
*/

namespace Symfony\AI\McpSdk\Capability\Prompt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Off topic, but what about using Mcp\Sdk as namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

No "Symfony" prefix?

That would be a first for Symfony organization.

I do see they are long an ugly, but it follows Symfony UX' pattern

chr-hertel added a commit that referenced this pull request Jun 6, 2025
chr-hertel added a commit that referenced this pull request Jun 13, 2025
@chr-hertel chr-hertel added the MCP SDK Issues & PRs about the MCP SDK label Jul 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

MCP SDK Issues & PRs about the MCP SDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants