Update new-command.md to two-generic options pattern#2707
Open
KarishmaGhiya wants to merge 1 commit into
Open
Conversation
Replace the legacy one-generic BaseCommand<TOptions> pattern with the new two-generic SubscriptionCommand<TOptions, TResult> pattern using [Option] attribute-driven binding throughout new-command.md. Key changes: - Options Class section: flat POCOs with [Option] attributes instead of inheritance hierarchies - Added Key Principles and Usage Patterns subsections covering 5 patterns (standard, mutually exclusive, enum/constrained, interface constraints, list/query) - Command Class section: two-generic base class, ISubscriptionResolver injection, ExecuteAsync takes TOptions directly - Base Service Command Classes: interface constraint pattern instead of RegisterOptions/BindOptions overrides - Unit Tests: SubscriptionCommandUnitTestsBase instead of CommandUnitTestsBase - Common Pitfalls and Best Practices: updated guidance Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
KarishmaGhiya
requested review from
alzimmermsft,
g2vinay,
hallipr,
joshfree,
saikoumudi,
tmeschter and
vukelich
Contributor
There was a problem hiding this comment.
Pull request overview
Updates servers/Azure.Mcp.Server/docs/new-command.md to document the newer two-generic SubscriptionCommand<TOptions, TResult> approach with attribute-driven option binding, replacing the legacy one-generic command/option-definition pattern.
Changes:
- Reworks the “Options Class” guidance to use flat POCOs and
OptionBinder-based binding with[Option]attributes. - Updates command examples to inject
ISubscriptionResolver, useExecuteAsync(CommandContext, TOptions, CancellationToken), and rely onValidateOptions. - Refreshes base-command and unit-test guidance to align with
SubscriptionCommandUnitTestsBase.
Invoking Livetests
Copilot submitted PRs are not trustworthy by default. Users with write access to the repo need to validate the contents of this PR before leaving a comment with the text /azp run mcp - pullrequest - live. This will trigger the necessary livetest workflows to complete required validation.
Comment on lines
+525
to
+528
| - Options classes are **flat POCOs** — no class inheritance. Each command has its own options class. | ||
| - `OptionBinder` discovers all `[Option]`-attributed properties on the concrete class and handles both registration (adding to the CLI parser) and binding (populating from parse results) automatically. | ||
| - **Required vs optional** is determined entirely by nullability: `required` keyword or non-nullable types = required; `?` = optional. No `.AsRequired()`/`.AsOptional()` calls needed. | ||
| - **No shared state**: Each command gets its own options instance per request — thread-safe by design. |
Comment on lines
+527
to
+528
| - **Required vs optional** is determined entirely by nullability: `required` keyword or non-nullable types = required; `?` = optional. No `.AsRequired()`/`.AsOptional()` calls needed. | ||
| - **No shared state**: Each command gets its own options instance per request — thread-safe by design. |
Comment on lines
+528
to
+530
| - **No shared state**: Each command gets its own options instance per request — thread-safe by design. | ||
| - **Implement `ISubscriptionOption`** if the command needs `Subscription`/`Tenant`. This enables `ISubscriptionResolver` post-processing. | ||
| - **Implement additional option interfaces** (e.g., `IStorageAccountOption`) only when base command classes need type-safe access to specific properties for shared behavior like validation. |
Comment on lines
727
to
733
| ```csharp | ||
| using System.Net; | ||
| using Azure.Mcp.Tools.{Toolset}.Models; | ||
| using Azure.Mcp.Tools.{Toolset}.Options; // REQUIRED: For {Toolset}OptionDefinitions | ||
| using Azure.Mcp.Tools.{Toolset}.Options.{Resource}; // For resource-specific options | ||
| using Azure.Mcp.Tools.{Toolset}.Options; // REQUIRED: For options classes | ||
| using Azure.Mcp.Tools.{Toolset}.Services; | ||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Mcp.Core.Commands; |
Comment on lines
+668
to
669
| public class BlobUploadOptions : ISubscriptionOption, IBlobOption | ||
| { |
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
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.
Replace the legacy one-generic BaseCommand pattern with the new two-generic SubscriptionCommand<TOptions, TResult> pattern using [Option] attribute-driven binding throughout new-command.md.
Key changes:
What does this PR do?
[Provide a clear, concise description of the changes][Add additional context, screenshots, or information that helps reviewers]GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline