Skip to content

Commit 4eaa06d

Browse files
Copilotagocke
andauthored
Fix parameter index not incrementing in nested subcommands (#32)
## Fix for Nested Subcommand Parameter Bug ✅ COMPLETE - [x] Identified the bug: `_paramIndex` is not incremented when parameters are matched - [x] Determine the best place to increment `_paramIndex` - [x] Implement the fix to increment `_paramIndex` after reading a parameter value - [x] Run the failing test to verify the fix works - [x] Run all tests to ensure no regressions - [x] Review and commit the changes - [x] Code review - no issues found - [x] Security scan - no vulnerabilities found - [x] Manual verification - confirmed fix works - [x] Refactored to remove side effects from CheckFields method - [x] Replaced tuple booleans with FieldKind enum for better clarity ## Summary Successfully fixed the bug where multiple command parameters in nested subcommands failed to parse correctly. The second parameter was being treated as a duplicate of the first, causing a "Duplicate key" error. ### Root Cause The `_paramIndex` field in `Deserializer.cs` was never incremented when a parameter was matched in the `CheckFields` method. This caused all parameters to match against ordinal 0, resulting in duplicate field assignments. ### Changes Made 1. **Test File** (`test/Serde.CmdLine.Test/SubCommandTests.cs`): - Added `NestedSubCommandWithTwoParameters` test case - Creates a `copy` subcommand with two parameters: `source` and `destination` - Tests parsing: `["copy", "source.txt", "dest.txt"]` 2. **Bug Fix** (`src/Serde.CmdLine/Deserializer.DeserializeType.cs`): - Modified `CheckFields` signature to return `(int fieldIndex, FieldKind fieldKind)` - Returns `FieldKind` enum indicating the type of field matched - Caller uses switch statement to increment appropriate index based on field kind - `CheckFields` is side-effect free, following established patterns 3. **New Type** (`src/Serde.CmdLine/OptionTypes.cs`): - Added `FieldKind` enum with values: None, Option, SubCommand, CommandGroup, Parameter - Each value is documented with its behavior regarding index increments - Provides clearer semantic meaning than tuple of booleans ### Verification ✅ **Unit Tests** - New test `NestedSubCommandWithTwoParameters` passes - All 21 CmdLine tests pass - All 4 Analyzer tests pass ✅ **Code Design** - `CheckFields` is side-effect free - Uses enum instead of tuple booleans for better readability and maintainability - Switch statement clearly maps field kinds to their required index increments - Follows the established pattern where method returns signals and caller handles state mutations <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/serdedotnet/serde.cmdline/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
1 parent 90fd272 commit 4eaa06d

File tree

3 files changed

+83
-9
lines changed

3 files changed

+83
-9
lines changed

src/Serde.CmdLine/Deserializer.DeserializeType.cs

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,19 @@ private int TryReadIndex(ISerdeInfo serdeInfo)
5555
continue;
5656
}
5757

58-
var (fieldIndex, incArgs) = CheckFields(arg);
58+
var (fieldIndex, fieldKind) = CheckFields(arg);
5959
if (fieldIndex >= 0)
6060
{
61-
if (incArgs)
61+
// Increment indices based on field type
62+
switch (fieldKind)
6263
{
63-
argIndex++;
64+
case FieldKind.Option:
65+
case FieldKind.SubCommand:
66+
argIndex++;
67+
break;
68+
case FieldKind.Parameter:
69+
_deserializer._paramIndex++;
70+
break;
6471
}
6572
return fieldIndex;
6673
}
@@ -156,21 +163,21 @@ int ITypeDeserializer.TryReadIndex(ISerdeInfo info)
156163
return null;
157164
}
158165

159-
private (int fieldIndex, bool incArgs) CheckFields(string arg)
166+
private (int fieldIndex, FieldKind fieldKind) CheckFields(string arg)
160167
{
161168
var cmd = _command;
162169
if (arg.StartsWith('-'))
163170
{
164171
var fieldIndex = CheckOptions(cmd, arg)?.FieldIndex ?? -1;
165-
return (fieldIndex, fieldIndex >= 0);
172+
return (fieldIndex, fieldIndex >= 0 ? FieldKind.Option : FieldKind.None);
166173
}
167174

168175
// Check for command group matches
169176
foreach (var subCmd in cmd.SubCommands)
170177
{
171178
if (arg == subCmd.Name)
172179
{
173-
return (subCmd.FieldIndex, true);
180+
return (subCmd.FieldIndex, FieldKind.SubCommand);
174181
}
175182
}
176183

@@ -180,7 +187,7 @@ int ITypeDeserializer.TryReadIndex(ISerdeInfo info)
180187
{
181188
if (name == arg)
182189
{
183-
return (cmdGroup.FieldIndex, false);
190+
return (cmdGroup.FieldIndex, FieldKind.CommandGroup);
184191
}
185192
}
186193

@@ -193,10 +200,10 @@ int ITypeDeserializer.TryReadIndex(ISerdeInfo info)
193200
// Parameters are positional, so we check the current param index
194201
if (_deserializer._paramIndex == param.Ordinal)
195202
{
196-
return (param.FieldIndex, false);
203+
return (param.FieldIndex, FieldKind.Parameter);
197204
}
198205
}
199-
return (-1, false);
206+
return (-1, FieldKind.None);
200207
}
201208

202209
public static Command ParseCommand(ISerdeInfo serdeInfo)

src/Serde.CmdLine/OptionTypes.cs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,30 @@
33

44
namespace Serde.CmdLine;
55

6+
internal enum FieldKind
7+
{
8+
/// <summary>
9+
/// No field matched
10+
/// </summary>
11+
None,
12+
/// <summary>
13+
/// An option field matched (requires incrementing arg index)
14+
/// </summary>
15+
Option,
16+
/// <summary>
17+
/// A subcommand field matched (requires incrementing arg index)
18+
/// </summary>
19+
SubCommand,
20+
/// <summary>
21+
/// A command group field matched (does not increment arg index)
22+
/// </summary>
23+
CommandGroup,
24+
/// <summary>
25+
/// A parameter field matched (requires incrementing param index)
26+
/// </summary>
27+
Parameter
28+
}
29+
630
internal record struct Option(
731
ImmutableArray<string> FlagNames,
832
int FieldIndex,

test/Serde.CmdLine.Test/SubCommandTests.cs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,4 +191,47 @@ public sealed partial record FirstCommand : SubCommand
191191
[Command("second")]
192192
public sealed partial record SecondCommand : SubCommand;
193193
}
194+
195+
/// <summary>
196+
/// Test to verify that two command parameters in a nested subcommand are parsed correctly.
197+
/// This test is designed to reproduce a bug where the second parameter is parsed as a
198+
/// duplicate of the first parameter due to _paramIndex not being incremented.
199+
/// </summary>
200+
[Fact]
201+
public void NestedSubCommandWithTwoParameters()
202+
{
203+
string[] testArgs = [ "copy", "source.txt", "dest.txt" ];
204+
var cmd = CmdLine.ParseRawWithHelp<CommandWithParams>(testArgs).Unwrap();
205+
Assert.Equal(new CommandWithParams
206+
{
207+
SubCommandWithParams = new SubCommandWithParams.CopyCommand
208+
{
209+
Source = "source.txt",
210+
Destination = "dest.txt"
211+
}
212+
}, cmd);
213+
}
214+
215+
[GenerateDeserialize]
216+
private partial record CommandWithParams
217+
{
218+
[CommandGroup("command")]
219+
public SubCommandWithParams? SubCommandWithParams { get; init; }
220+
}
221+
222+
[GenerateDeserialize]
223+
private abstract partial record SubCommandWithParams
224+
{
225+
private SubCommandWithParams() { }
226+
227+
[Command("copy")]
228+
public sealed partial record CopyCommand : SubCommandWithParams
229+
{
230+
[CommandParameter(0, "source")]
231+
public string? Source { get; init; }
232+
233+
[CommandParameter(1, "destination")]
234+
public string? Destination { get; init; }
235+
}
236+
}
194237
}

0 commit comments

Comments
 (0)