Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
18 changes: 11 additions & 7 deletions src/Serde.CmdLine/Deserializer.DeserializeType.cs
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,17 @@ private int TryReadIndex(ISerdeInfo serdeInfo)
continue;
}

var (fieldIndex, incArgs) = CheckFields(arg);
var (fieldIndex, incArgs, isParameter) = CheckFields(arg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the additional parameter to this tuple getting a bit messy? The method basically wants to return an index, then an indication of what type of field was returned, so we can decide whether or not to increment args, parameter index, etc. Would an enum value be better suited for this? @copilot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, an enum is much clearer. I've refactored to use a FieldKind enum with values: None, Option, SubCommand, CommandGroup, and Parameter. The method now returns (int fieldIndex, FieldKind fieldKind) and the caller uses a switch statement to determine which index to increment. This is much more readable and maintainable than the tuple of booleans.

Commit: 215bbe9

if (fieldIndex >= 0)
{
if (incArgs)
{
argIndex++;
}
if (isParameter)
{
_deserializer._paramIndex++;
}
return fieldIndex;
}

Expand Down Expand Up @@ -156,21 +160,21 @@ int ITypeDeserializer.TryReadIndex(ISerdeInfo info)
return null;
}

private (int fieldIndex, bool incArgs) CheckFields(string arg)
private (int fieldIndex, bool incArgs, bool isParameter) CheckFields(string arg)
{
var cmd = _command;
if (arg.StartsWith('-'))
{
var fieldIndex = CheckOptions(cmd, arg)?.FieldIndex ?? -1;
return (fieldIndex, fieldIndex >= 0);
return (fieldIndex, fieldIndex >= 0, false);
}

// Check for command group matches
foreach (var subCmd in cmd.SubCommands)
{
if (arg == subCmd.Name)
{
return (subCmd.FieldIndex, true);
return (subCmd.FieldIndex, true, false);
}
}

Expand All @@ -180,7 +184,7 @@ int ITypeDeserializer.TryReadIndex(ISerdeInfo info)
{
if (name == arg)
{
return (cmdGroup.FieldIndex, false);
return (cmdGroup.FieldIndex, false, false);
}
}

Expand All @@ -193,10 +197,10 @@ int ITypeDeserializer.TryReadIndex(ISerdeInfo info)
// Parameters are positional, so we check the current param index
if (_deserializer._paramIndex == param.Ordinal)
{
return (param.FieldIndex, false);
return (param.FieldIndex, false, true);
}
}
return (-1, false);
return (-1, false, false);
}

public static Command ParseCommand(ISerdeInfo serdeInfo)
Expand Down
43 changes: 43 additions & 0 deletions test/Serde.CmdLine.Test/SubCommandTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -191,4 +191,47 @@ public sealed partial record FirstCommand : SubCommand
[Command("second")]
public sealed partial record SecondCommand : SubCommand;
}

/// <summary>
/// Test to verify that two command parameters in a nested subcommand are parsed correctly.
/// This test is designed to reproduce a bug where the second parameter is parsed as a
/// duplicate of the first parameter due to _paramIndex not being incremented.
/// </summary>
[Fact]
public void NestedSubCommandWithTwoParameters()
{
string[] testArgs = [ "copy", "source.txt", "dest.txt" ];
var cmd = CmdLine.ParseRawWithHelp<CommandWithParams>(testArgs).Unwrap();
Assert.Equal(new CommandWithParams
{
SubCommandWithParams = new SubCommandWithParams.CopyCommand
{
Source = "source.txt",
Destination = "dest.txt"
}
}, cmd);
}

[GenerateDeserialize]
private partial record CommandWithParams
{
[CommandGroup("command")]
public SubCommandWithParams? SubCommandWithParams { get; init; }
}

[GenerateDeserialize]
private abstract partial record SubCommandWithParams
{
private SubCommandWithParams() { }

[Command("copy")]
public sealed partial record CopyCommand : SubCommandWithParams
{
[CommandParameter(0, "source")]
public string? Source { get; init; }

[CommandParameter(1, "destination")]
public string? Destination { get; init; }
}
}
}