Skip to content
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

-v flag conflict between additional commands and default command #1479

Open
BCoskun opened this issue Feb 29, 2024 · 5 comments · May be fixed by #1664
Open

-v flag conflict between additional commands and default command #1479

BCoskun opened this issue Feb 29, 2024 · 5 comments · May be fixed by #1664
Assignees
Labels
area-CLI Command-Line Interface bug Something isn't working ⭐ top bug Top bug.
Milestone

Comments

@BCoskun
Copy link

BCoskun commented Feb 29, 2024

Information

  • V0.47 and 0.48 behavior change for -v flag.

Describe the bug
I was using -v short hand flag for sub command and it was working until V0.48 . With Version 0.48 parsing behavior changed and when we use -v for subcommand, it print application version directly.

To Reproduce
With Version 0.47 ConsoleApp.exe -v and ConsoleApp.exe subcommand -v has different behavior.
With Version 0.48 ConsoleApp.exe -v and ConsoleApp.exe subcommand -v has same behavior.

Expected behavior
The sub command -v should be parsed for the sub command context


Please upvote 👍 this issue if you are interested in it.

@BCoskun BCoskun added bug Something isn't working needs triage labels Feb 29, 2024
@github-project-automation github-project-automation bot moved this to Todo 🕑 in Spectre Console Feb 29, 2024
@JKamsker
Copy link

Can you provide an unit test for this?
This would help understanding the problem and having a definition of "done"

Similar bug in the past #1400
My issue probably caused this change 😅

I guess there is no absolute right answer, and if it is it is harder to do than it may first look like

@FrankRay78
Copy link
Contributor

I introduced the bug @JKamsker, unfortunately. I knew it was a breaking change, and flagged it as such, but I think the change in functionality is probably now best described as a regression. I'll add this to my stack to review/address.

#1400 (comment)

And

FYI. I've submitted the PR above to revert this to existing behaviour @JKamsker

#1400 (comment)

Background
Apologies for creating this problem. The above issue description is exactly correct, we changed this behaviour between 0.47 and 0.48.

It had worked as per the current 0.48 earlier than 0.47, then I did some refactoring, changed it to the behaviour in 0.47, flagged it as a breaking change (thinking it was fine to make breaking changes, if warranted, as we are still < version 1.0), which was accepted by the maintainer team and merged.

Then we found a number of unhappy users of spectre.console (because we broke their apps), and so on further consideration, we reverted the change. Unfortunately, I doubt we'd be looking to revert it once again.

@FrankRay78 FrankRay78 added area-CLI Command-Line Interface and removed needs triage labels Mar 7, 2024
@FrankRay78 FrankRay78 self-assigned this Mar 7, 2024
@github-actions github-actions bot added the ⭐ top bug Top bug. label Apr 15, 2024
@FrankRay78
Copy link
Contributor

This appears fixed in 0.49.1 @BCoskun

I've created the following console application to test the behaviour:

using Spectre.Console;
using Spectre.Console.Cli;
using System.ComponentModel;

public class HelloCommandSettings : CommandSettings
{
    [CommandOption("-v|--version")]
    [DefaultValue("Hello world")]
    public string Message { get; set; }
}

public class HelloCommand : Command<HelloCommandSettings>
{
    public override int Execute(CommandContext context, HelloCommandSettings settings)
    {
        AnsiConsole.MarkupLine(settings.Message);
        return 0;
    }
}

public static class Program
{
    public static int Main(string[] args)
    {
        var app = new CommandApp();

        app.Configure(config =>
        {
            config.AddCommand<HelloCommand>("hello");
        });

        return app.Run(args);
    }

Behaviour under different spectre.console versions

0.47

image

0.48

image

0.49.1

image

@FrankRay78
Copy link
Contributor

There is still a bug, however, as when I add in the application version using the following configuration line:

            config.SetApplicationVersion("1.0");

The help text for the hello command erroneously includes the -v, --version associated at the application root:

image

At least the application behaviour is now functioning correctly.

@FrankRay78 FrankRay78 added this to the 1.0 milestone Aug 12, 2024
@FrankRay78 FrankRay78 linked a pull request Aug 13, 2024 that will close this issue
@FrankRay78
Copy link
Contributor

FrankRay78 commented Aug 15, 2024

Hey @JKamsker, if you have a minute, I'd love to hear your thoughts on the following test I'm authoring to address this issue, particularly the explanation given in the summary:

            /// <summary>
            /// When a command with a version flag in the settings is set as the application default command,
            /// then override the in-built Application Version flag with the command version flag instead.
            /// Rationale: This behaviour makes the most sense because the other flags for the default command
            /// will be shown in the help output and the user can set any of these when executing the application.
            /// </summary>
            [Theory]
            [InlineData("-?")]
            [InlineData("-h")]
            [InlineData("--help")]
            public void Help_Should_Include_Command_Version_Flag_For_Default_Command(string helpOption)
            {
                // Given
                var fixture = new CommandAppTester();
                fixture.SetDefaultCommand<Spectre.Console.Tests.Data.VersionCommand>();
                fixture.Configure(configurator =>
                {
                    configurator.SetApplicationVersion("1.0");
                });

                // When
                var result = fixture.Run(helpOption);

                // Then
                result.Output.ShouldContain("-v, --version    The command version");
                result.Output.ShouldNotContain("-v, --version    Prints version information");
            }

@FrankRay78 FrankRay78 linked a pull request Oct 5, 2024 that will close this issue
6 tasks
@FrankRay78 FrankRay78 linked a pull request Oct 9, 2024 that will close this issue
6 tasks
@FrankRay78 FrankRay78 linked a pull request Oct 9, 2024 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment