Skip to content

Symbol.Description does not need to be virtual #2045

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

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Symbol.Description does not need to be virtual #2045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03ccb99
Symbol.Description does not need to be virtual, we can pass the descr…
adamsitnik Feb 14, 2023
a60da9c
Merge remote-tracking branch 'upstream/main' into nonVirtualDescription
adamsitnik Feb 18, 2023
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: 3 additions & 3 deletions src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ void Default(Exception exception, InvocationContext context)
/// <returns>The same instance of <see cref="CommandLineBuilder"/>.</returns>
public static CommandLineBuilder UseHelp(this CommandLineBuilder builder, int? maxWidth = null)
{
return builder.UseHelp(new HelpOption(() => builder.LocalizationResources), maxWidth);
return builder.UseHelp(new HelpOption(builder.LocalizationResources), maxWidth);
Copy link

Choose a reason for hiding this comment

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

If the application first calls CommandLineBuilderExtensions.UseDefaults and then calls CommandLineBuilderExtensions.UseLocalizationResources, this PR will prevent the help from using the application-specified resources.

Copy link

Choose a reason for hiding this comment

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

I suppose the application could fix that by calling CommandLineBuilderExtensions.UseLocalizationResources before CommandLineBuilderExtensions.UseDefaults. But if there is such a constraint, then it should be mentioned in the documentation of CommandLineBuilderExtensions.UseLocalizationResources.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is correct, but we are soon going to make LocalizationResources internal and never expose it again (#2041).

Copy link

Choose a reason for hiding this comment

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

#1915 still "needs discussion".

Copy link
Contributor

Choose a reason for hiding this comment

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

I had meant to comment on this last week. I've updated #1915 with some thoughts on this.

}

/// <summary>
Expand All @@ -250,7 +250,7 @@ public static CommandLineBuilder UseHelp(
this CommandLineBuilder builder,
params string[] helpAliases)
{
return builder.UseHelp(new HelpOption(helpAliases, () => builder.LocalizationResources));
return builder.UseHelp(new HelpOption(helpAliases, builder.LocalizationResources));
}

/// <summary>
Expand All @@ -270,7 +270,7 @@ public static CommandLineBuilder UseHelp(

if (builder.HelpOption is null)
{
builder.UseHelp(new HelpOption(() => builder.LocalizationResources), maxWidth);
builder.UseHelp(new HelpOption(builder.LocalizationResources), maxWidth);
}

return builder;
Expand Down
18 changes: 4 additions & 14 deletions src/System.CommandLine/Help/HelpOption.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,21 @@ namespace System.CommandLine.Help
{
internal class HelpOption : Option<bool>
{
private readonly Func<LocalizationResources> _localizationResources;
private string? _description;

public HelpOption(string[] aliases, Func<LocalizationResources> getLocalizationResources)
: base(aliases, null, new Argument<bool> { Arity = ArgumentArity.Zero })
internal HelpOption(string[] aliases, LocalizationResources localizationResources)
: base(aliases, localizationResources.HelpOptionDescription(), new Argument<bool> { Arity = ArgumentArity.Zero })
{
_localizationResources = getLocalizationResources;
AppliesToSelfAndChildren = true;
}

public HelpOption(Func<LocalizationResources> getLocalizationResources) : this(new[]
internal HelpOption(LocalizationResources localizationResources) : this(new[]
{
"-h",
"/h",
"--help",
"-?",
"/?"
}, getLocalizationResources)
{
}

public override string? Description
}, localizationResources)
{
get => _description ??= _localizationResources().HelpOptionDescription();
set => _description = value;
}

internal override bool IsGreedy => false;
Expand Down
19 changes: 4 additions & 15 deletions src/System.CommandLine/Help/VersionOption.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,15 @@ namespace System.CommandLine.Help
{
internal class VersionOption : Option<bool>
{
private readonly CommandLineBuilder _builder;
private string? _description;

public VersionOption(CommandLineBuilder builder) : base("--version", null, new Argument<bool> { Arity = ArgumentArity.Zero })
internal VersionOption(CommandLineBuilder builder)
: base("--version", builder.LocalizationResources.VersionOptionDescription(), new Argument<bool> { Arity = ArgumentArity.Zero })
{
_builder = builder;

AddValidators();
}

public VersionOption(string[] aliases, CommandLineBuilder builder) : base(aliases)
internal VersionOption(string[] aliases, CommandLineBuilder builder)
: base(aliases, builder.LocalizationResources.VersionOptionDescription())
{
_builder = builder;

AddValidators();
}

Expand All @@ -50,12 +45,6 @@ private static bool IsNotImplicit(SymbolResult symbolResult)
};
}

public override string? Description
{
get => _description ??= _builder.LocalizationResources.VersionOptionDescription();
set => _description = value;
}

internal override bool IsGreedy => false;

public override bool Equals(object? obj) => obj is VersionOption;
Expand Down
2 changes: 1 addition & 1 deletion src/System.CommandLine/Symbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ private protected Symbol()
/// <summary>
/// Gets or sets the description of the symbol.
/// </summary>
public virtual string? Description { get; set; }
public string? Description { get; set; }

/// <summary>
/// Gets or sets the name of the symbol.
Expand Down