Skip to content

Fix #420 - Check for value token before expanding enumerable types with separator char #672

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

wo-roberts
Copy link

No description provided.

@rmunn
Copy link
Contributor

rmunn commented Aug 20, 2020

I don't think this is quite the right solution. I think in addition to checking if the token to be exploded is a Value, we also need to check if the previous token is a Name. I'll try to create a unit test that proves that.

BTW, I'm currently working on a fix for #396 that looks like it's going to end up fixing #420 at the same time, so once that PR is submitted I'll link to it from this one as well.

@rmunn
Copy link
Contributor

rmunn commented Aug 20, 2020

Here's a unit test that fails with this PR, but works if you also check that the previous token is a Name:

[Fact]
public static void Values_with_same_name_as_sequence_option_do_not_cause_later_values_to_split_on_separators()
{
    var args = new[] { "c", "x,y" };
    var tokensExpected = new[] { Token.Value("c"), Token.Value("x,y") };
    var typeInfo = typeof(OptionsWithSeparator);

    var specProps = typeInfo.GetSpecifications(pi => SpecificationProperty.Create(
            Specification.FromProperty(pi), pi, Maybe.Nothing<object>()))
        .Select(sp => sp.Specification)
        .OfType<OptionSpecification>();

    var tokenizerResult = Tokenizer.ConfigureTokenizer(StringComparer.InvariantCulture, false, false)(args, specProps);
    var tokens = tokenizerResult.SucceededWith();
    tokens.Should().BeEquivalentTo(tokensExpected);
}

internal class OptionsWithSeparator
{
        [Option('f', "flag", HelpText = "Flag")]
        public bool Flag { get; set; }

        [Option('c', "categories", Required = false, Separator = ',', HelpText = "Categories")]
        public IEnumerable<string> Categories { get; set; }

        [Option('j', "jobId", Required = true, HelpText = "Texts.ExplainJob")]
        public int JobId { get; set; }
}

@rmunn
Copy link
Contributor

rmunn commented Aug 20, 2020

47618e0 (part of #684) is a better fix, which fixes the unit test in #672 (comment) that fails with this PR.

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.

2 participants