-
Notifications
You must be signed in to change notification settings - Fork 5k
stop using System.CommandLine types that won't be public anymore #116065
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces obsolete System.CommandLine
types by switching help extensions to use ParseResult
and Action<ParseResult>
instead of HelpContext
and IEnumerable<Func<HelpContext, bool>>
. It also centralizes the post-build help customization into a new CustomizedHelpAction
.
- Change
GetExtendedHelp
in three root commands tovoid GetExtendedHelp(ParseResult)
and remove legacyHelpContext
-based enumerator. - Update
UseExtendedHelp
extension to accept anAction<ParseResult>
and wire upCustomizedHelpAction
. - Add
CustomizedHelpAction
implementation and an unusedusing
import inCommandLineHelpers.cs
.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/coreclr/tools/dotnet-pgo/PgoRootCommand.cs | Updated GetExtendedHelp signature and logic to print examples for trace |
src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs | Updated GetExtendedHelp signature and removed legacy layout iteration |
src/coreclr/tools/aot/ILCompiler/ILCompilerRootCommand.cs | Updated GetExtendedHelp signature and removed legacy layout iteration |
src/coreclr/tools/Common/CommandLineHelpers.cs | Changed UseExtendedHelp to use CustomizedHelpAction and added helper |
Comments suppressed due to low confidence (2)
src/coreclr/tools/Common/CommandLineHelpers.cs:133
- [nitpick] The parameter name
customizer
is clear, but the unused discard_
in the root commands may be confusing. Consider using a descriptive name likeparseResult
for consistency.
public static RootCommand UseExtendedHelp(this RootCommand command, Action<ParseResult> customizer)
src/coreclr/tools/dotnet-pgo/PgoRootCommand.cs:263
- The new
GetExtendedHelp(ParseResult)
behavior (especially forcreate-mibc
andcreate-jittrace
) lacks automated tests to verify the extended help output.
public static void GetExtendedHelp(ParseResult parseResult)
Co-authored-by: Copilot <[email protected]>
@@ -329,64 +329,57 @@ public ILCompilerRootCommand(string[] args) : base(".NET Native IL Compiler") | |||
}); | |||
} | |||
|
|||
public static IEnumerable<Func<HelpContext, bool>> GetExtendedHelp(HelpContext _) | |||
public static void GetExtendedHelp(ParseResult _) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not returning anything after this change, so Get...
name does not fit what it does anymore.
I would rename it to PrintHelp
, DisplayHelp
or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I have not commented on all places with this name. All of them should be fixed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo a few nits
Context: dotnet/command-line-api#2563