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

Verb aliases #636

Merged
merged 5 commits into from
Jun 15, 2020
Merged

Conversation

johnjaylward
Copy link
Contributor

@johnjaylward johnjaylward commented May 26, 2020

Fixes #6
Fixes #517

@johnjaylward
Copy link
Contributor Author

I wasn't sure how best to update the Help messages generated to indicate the new alias support. If anyone has suggestions for what they'd like to see and I can update

@moh-hassan
Copy link
Collaborator

moh-hassan commented May 27, 2020

Update helptext here
Just add ' alias names

@moh-hassan
Copy link
Collaborator

Thanks for your work and fixing #6

@johnjaylward
Copy link
Contributor Author

Help messages updated and tested. Let me know if you think any changes are needed.

@moh-hassan moh-hassan added this to the 2.9 milestone May 30, 2020
@moh-hassan moh-hassan changed the base branch from master to develop June 15, 2020 15:20
@moh-hassan moh-hassan changed the base branch from develop to master June 15, 2020 15:56
@moh-hassan
Copy link
Collaborator

moh-hassan commented Jun 15, 2020

@johnjaylward
Please, rebase PR to develop branch and resolve confliction.
Also, for verb test case, set isDefault: false or remove this line. IsDefault means don't pass verb in the commandline.

@johnjaylward johnjaylward changed the base branch from master to develop June 15, 2020 16:38
@johnjaylward
Copy link
Contributor Author

@moh-hassan , I finished the rebase, but I don't think I need to remove the "Default Verb" setting. I set up the test case purposefully to take into account a default verb. If I remove that option, I will need to adjust my test cases accordingly.

@johnjaylward
Copy link
Contributor Author

@moh-hassan , I ended up making a few more test cases which show that it work both with a default verb and without a default verb. Also added a few comments to show intention of the tests.

@moh-hassan moh-hassan merged commit 0154c98 into commandlineparser:develop Jun 15, 2020
@johnjaylward johnjaylward deleted the VerbAliases branch June 15, 2020 20:14
@twaalewijn
Copy link

@moh-hassan I'm working on a PR to introduce sub verbs into the library.
After I rebased/merged that work onto the latest develop several of my tests with help output started failing because certain lines we're being indented/padded differently than before.

Changing the call to OptionSpecification.NewSwitch that was changed in HelpText in this PR to once again pass string.Empty as the short name and concatenating the name and aliases into the long name seems to fix it.

E.g. something like this

OptionSpecification.NewSwitch(
    string.Empty,
    new[] { verbTuple.Item1.Name }.Concat(verbTuple.Item1.Aliases).ToDelimitedString(", "),
    false,
    verbTuple.Item1.IsDefault ? "(Default Verb) " + verbTuple.Item1.HelpText : verbTuple.Item1.HelpText,  //Default verb
    string.Empty,
    verbTuple.Item1.Hidden);

Just thought I'd mention it before 2.9 is released in case it is a problem.

@moh-hassan
Copy link
Collaborator

@twaalewijn
I couldn't find OptionSpecification.NewSwitch in the commits of this PR.
Can you point me to the location of this change in the commits of PR?

several of my tests with help output started failing because certain lines we're being indented/padded differently than before.

Can this may to related to the Configuration of Git to handle line endings?

Can Your suggestion cause some tests to fail?

@twaalewijn
Copy link

@moh-hassan
This is the call I am referring to: https://github.com/commandlineparser/commandline/pull/636/files#diff-88c7f4706542a25c9d5ec8b7a7b49bd5R853

As can be seen in the diff the first parameter (short name) used to be string.Empty and the verb name was passed as the second parameter (long name).

After passing both the verb name and possible aliases to the long name parameter as shown in my earlier example my expected help text was indented/aligned like it was before this PR.

Can this may to related to the Configuration of Git to handle line endings?

I guess I cannot 100% rule that out but I do not think so.
Sorry if I did not explain it clearly enough, I'm talking about the spacing in the help that is added between the verb and help text:

help <this added space> Display more information on a specific command.

Can Your suggestion cause some tests to fail?

It will cause these instances of theory tests to fail:

But these tests are expecting wrong outputs I think.

It isn't very easy to see in the InlineData of lines 163 and 170 but in the data of lines 88 and 96 you can see that the help text for delete, help and version are not lining up correctly with copy, cp, cpy.

The data of line 88 for example currently looks like this:

[InlineData("--help", true, new string[]
    {
        "copy, cp, cpy    (Default Verb) Copy some stuff",
        "move, mv",
        "delete        Delete stuff",
        "help          Display more information on a specific command.",
        "version       Display version information.",
    })]

But should have looked like this instead:

[InlineData("--help", true, new string[]
    {
        "copy, cp, cpy    (Default Verb) Copy some stuff",
        "move, mv",
        "delete           Delete stuff",
        "help             Display more information on a specific command.",
        "version          Display version information.",
    })]

@johnjaylward
Copy link
Contributor Author

@twaalewijn, I see. The reason I swapped the verb to the "short" position was to ensure it would always be first in the list. When the verb was in the "long" position, it changed the order to have all the aliases first, which I did not like.

The Issue6Tests were generated for this PR. I'll look at opening a new PR to switch the order back, but that will likely change the help to read like this:

[InlineData("--help", true, new string[]
    {
        "cp, cpy, copy    (Default Verb) Copy some stuff",
        "mv, move",
        "delete           Delete stuff",
        "help             Display more information on a specific command.",
        "version          Display version information.",
    })]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verb aliases
3 participants