chore(LLMs): add output parameter #896
Merged
Conversation
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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a configurable output folder parameter to the LLMs docs generator tool and adjusts documentation generation paths to use it instead of a hard‑coded publish path. Sequence diagram for LLMs docs generator CLI with output parametersequenceDiagram
actor Developer
participant CLI as ArgumentsHelper
participant RootCommand
participant DocsGenerator
Developer->>CLI: Parse(args)
CLI->>RootCommand: Configure rootFolderOption and outputFolderOption
Developer->>RootCommand: Execute with --root and --output
RootCommand->>RootCommand: SetAction callback invoked
RootCommand->>RootCommand: GetValue(rootFolderOption)
alt rootFolder is null or empty
RootCommand-->>Developer: Exit without generating docs
else rootFolder is valid
RootCommand->>RootCommand: GetValue(outputFolderOption)
alt outputFolder is null or empty
RootCommand-->>Developer: Exit without generating docs
else outputFolder is valid
RootCommand->>DocsGenerator: GenerateAllAsync(rootFolder, outputFolder)
DocsGenerator-->>RootCommand: Generation completed
RootCommand-->>Developer: Return success
end
end
Class diagram for updated LLMs docs generator arguments and generatorclassDiagram
class ArgumentsHelper {
+static ParseResult Parse(string[] args)
}
class DocsGenerator {
+static Task GenerateAllAsync(string rootFolder, string outputFolder)
}
class RootCommand {
+RootCommand(string description)
+void SetAction(Func<InvocationContext, Task> action)
+ParseResult Parse(string[] args)
}
class Option_string_nullable_ {
+Option_string_nullable_(string alias)
+string Description
}
class ParseResult {
}
class InvocationContext {
+T GetValue~T~(Option_T_ option)
}
ArgumentsHelper ..> RootCommand : uses
ArgumentsHelper ..> Option_string_nullable_ : uses
ArgumentsHelper ..> ParseResult : returns
ArgumentsHelper ..> InvocationContext : action parameter
InvocationContext ..> Option_string_nullable_ : uses
RootCommand ..> ParseResult : returns
DocsGenerator <.. ArgumentsHelper : called by
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
ArgumentsHelper, returning silently when--outputis missing makes the tool harder to use; consider either keeping the previous default publish path when--outputis not supplied or emitting a clear error/help message instead of just returning. - The new
--outputoption description is a bit ambiguous given thatDocsGeneratorappendswwwroot/llms; it may be clearer to describe it explicitly as the publish root (or similar) so users know what directory level to pass. - When using the provided
outputFolderinDocsGenerator, consider validating/creating the target directory and logging if it doesn't exist, so failures are explicit rather than resulting in downstream IO errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ArgumentsHelper`, returning silently when `--output` is missing makes the tool harder to use; consider either keeping the previous default publish path when `--output` is not supplied or emitting a clear error/help message instead of just returning.
- The new `--output` option description is a bit ambiguous given that `DocsGenerator` appends `wwwroot/llms`; it may be clearer to describe it explicitly as the publish root (or similar) so users know what directory level to pass.
- When using the provided `outputFolder` in `DocsGenerator`, consider validating/creating the target directory and logging if it doesn't exist, so failures are explicit rather than resulting in downstream IO errors.
## Individual Comments
### Comment 1
<location> `tools/BootstrapBlazor.LLMsDocsGenerator/ArgumentsHelper.cs:15-23` </location>
<code_context>
public static ParseResult Parse(string[] args)
{
var rootFolderOption = new Option<string?>("--root") { Description = "Set the root folder of project" };
+ var outputFolderOption = new Option<string?>("--output") { Description = "Set the publish folder of project" };
var rootCommand = new RootCommand("BootstrapBlazor LLMs Documentation Generator")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider making `--root`/`--output` required or surfacing an error instead of silently returning.
Right now the command effectively requires both `rootFolder` and `outputFolder`, but just `return`s if either is missing, so the user gets no feedback. Instead, either declare these options as required in the command configuration or emit an explicit error and non‑zero exit code when one is omitted, so a call like `llms-docs-generator --root ...` without `--output` doesn’t silently do nothing.
Suggested implementation:
```csharp
public static ParseResult Parse(string[] args)
{
var rootFolderOption = new Option<string>("--root")
{
Description = "Set the root folder of project",
IsRequired = true
};
var outputFolderOption = new Option<string>("--output")
{
Description = "Set the publish folder of project",
IsRequired = true
};
var rootCommand = new RootCommand("BootstrapBlazor LLMs Documentation Generator")
{
rootFolderOption,
outputFolderOption
};
rootCommand.SetAction(async result =>
{
var rootFolder = result.GetValueForOption(rootFolderOption);
var outputFolder = result.GetValueForOption(outputFolderOption);
// existing logic that previously ran after the null-checks goes here,
// now guaranteed to have non-null rootFolder and outputFolder
});
```
I can only see part of the `SetAction` body. To fully implement the behavior without silent returns you should:
1. Remove any `if (rootFolder is null) { return; }` / `if (outputFolder is null) { return; }` guards that existed before.
2. Move the existing logic that used `rootFolder`/`outputFolder` into the new action body after the `GetValueForOption` calls.
3. If you want more user-friendly output in addition to the built-in System.CommandLine error for missing required options, you can also:
- Add examples to the command description, or
- Use `rootCommand.TreatUnmatchedTokensAsErrors = true;` if not already set, to ensure bad invocations fail with non-zero exit codes.
With `IsRequired = true`, invocations like `llms-docs-generator --root ...` (without `--output`) will now emit an explicit error and a non-zero exit code instead of silently doing nothing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds a new --output parameter to the LLMs documentation generator tool, allowing users to specify a custom output folder instead of using a hardcoded path. The change replaces the previously hardcoded output path (bin/Release/net10.0/publish/wwwroot/llms) with a configurable parameter and bumps the tool version from 10.0.1 to 10.0.2.
Key changes:
- Added
--outputcommand-line parameter to specify the publish folder - Modified
GenerateAllAsyncto accept an output folder parameter - Updated path construction to use the configurable output folder
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tools/BootstrapBlazor.LLMsDocsGenerator/DocsGenerator.cs | Added outputFolder parameter to GenerateAllAsync method and updated output path construction |
| tools/BootstrapBlazor.LLMsDocsGenerator/ArgumentsHelper.cs | Added --output option definition and validation logic for the new parameter |
| tools/BootstrapBlazor.LLMsDocsGenerator/BootstrapBlazor.LLMsDocsGenerator.csproj | Incremented version from 10.0.1 to 10.0.2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static ParseResult Parse(string[] args) | ||
| { | ||
| var rootFolderOption = new Option<string?>("--root") { Description = "Set the root folder of project" }; | ||
| var outputFolderOption = new Option<string?>("--output") { Description = "Set the publish folder of project" }; |
There was a problem hiding this comment.
The outputFolderOption should be marked as required since the code will silently fail if it's not provided. Without making this parameter required, users won't get proper command-line help indicating this is a mandatory parameter. Consider adding .IsRequired = true to the option definition to make the requirement explicit and provide better CLI usability.
Suggested change
| var outputFolderOption = new Option<string?>("--output") { Description = "Set the publish folder of project" }; | |
| var outputFolderOption = new Option<string?>("--output") { Description = "Set the publish folder of project", IsRequired = true }; |
|
|
||
| internal static class DocsGenerator | ||
| { | ||
| /// <summary> |
There was a problem hiding this comment.
This change introduces a breaking API change by making the output folder parameter mandatory, but the existing documentation in README.md shows that the tool can be called without parameters (e.g., 'llms-docs' or 'dotnet run --project tools/LlmsDocsGenerator'). These examples will no longer work with the new changes. The documentation should be updated to reflect that both --root and --output parameters are now required, or the parameters should be made optional with sensible defaults to maintain backward compatibility.
Suggested change
| /// <summary> | |
| /// <summary> | |
| /// Generate all documentation files using the current directory | |
| /// as both the root and output folder. | |
| /// </summary> | |
| public static Task GenerateAllAsync() | |
| { | |
| var currentDirectory = Directory.GetCurrentDirectory(); | |
| return GenerateAllAsync(currentDirectory, currentDirectory); | |
| } | |
| /// <summary> | |
| /// Generate all documentation files using the specified root folder | |
| /// as both the source root and the output root. | |
| /// </summary> | |
| public static Task GenerateAllAsync(string rootFolder) | |
| { | |
| return GenerateAllAsync(rootFolder, rootFolder); | |
| } | |
| /// <summary> |
| { | ||
| public static ParseResult Parse(string[] args) | ||
| { | ||
| var rootFolderOption = new Option<string?>("--root") { Description = "Set the root folder of project" }; |
There was a problem hiding this comment.
The rootFolderOption should be marked as required since the code will silently fail if it's not provided. Without making this parameter required, users won't get proper command-line help indicating this is a mandatory parameter. Consider adding .IsRequired = true to the option definition to make the requirement explicit and provide better CLI usability.
Suggested change
| var rootFolderOption = new Option<string?>("--root") { Description = "Set the root folder of project" }; | |
| var rootFolderOption = new Option<string?>("--root") | |
| { | |
| Description = "Set the root folder of project", | |
| IsRequired = true | |
| }; |
Comment on lines
+31
to
+35
| var outputFolder = result.GetValue(outputFolderOption); | ||
| if (string.IsNullOrEmpty(outputFolder)) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
When the output folder parameter is missing or empty, the function silently returns without providing any feedback to the user. This creates a poor user experience as the user won't know why the tool didn't execute. Consider logging an error message or providing usage information before returning, similar to how other command-line tools behave when required parameters are missing.
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.
Link issues
fixes #895
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a configurable output folder to the LLMs documentation generator tool and update documentation generation paths accordingly.
New Features:
Enhancements: