Skip to content

Commit d92410d

Browse files
[ListSort, Polish] Compile-time drift guard, code cleanup, and docs
- Add static_assert on SortField::Available at enum definition site to catch new values that bypass constructor handling. - Replace invalid-sort assertion with FAIL_FAST_MSG for clearer diagnostics. - Collapse two consecutive sortFields.empty() checks into a single branch. - Format SortablePackageEntry constructor arguments one-per-line. - Add --query example to list.md Example queries section.
1 parent 924f4c5 commit d92410d

5 files changed

Lines changed: 82 additions & 26 deletions

File tree

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -485,40 +485,39 @@ namespace AppInstaller::CLI::Workflow
485485
}
486486
else
487487
{
488-
// Invalid values should not reach here; ValidateArgumentsInternal
489-
// rejects them with an error before workflow execution begins.
490-
THROW_HR(E_UNEXPECTED);
488+
// Invalid values should not reach here; ValidateArguments
489+
// rejects them with a CommandException before workflow execution begins.
490+
FAIL_FAST_MSG("Unexpected sort field value reached workflow; validation should have caught this.");
491491
}
492492
}
493493
}
494494
else
495495
{
496496
sortFields = User().Get<Setting::OutputSortOrder>();
497497

498-
// When the free-text query argument is present and the user has NOT
499-
// configured a sort preference in settings, preserve relevance ordering.
500-
// Only the positional query argument populates searchRequest.Query and
501-
// produces meaningful relevance ranking; filter arguments like --id,
502-
// --name, --tag etc. use exact/substring matching where all results
503-
// have equivalent relevance.
504-
// If the user explicitly configured output.sortOrder, respect it even
505-
// with queries — that is an explicit user preference.
506-
if (sortFields.empty() && context.Args.Contains(Execution::Args::Type::Query))
507-
{
508-
return;
509-
}
510-
511-
// No settings configured and no query — apply default sort by name
512-
// so that output is deterministic and user-friendly.
513498
if (sortFields.empty())
514499
{
500+
if (context.Args.Contains(Execution::Args::Type::Query))
501+
{
502+
// When the free-text query argument is present and the user has NOT
503+
// configured a sort preference in settings, preserve relevance ordering.
504+
// Only the positional query argument populates searchRequest.Query and
505+
// produces meaningful relevance ranking; filter arguments like --id,
506+
// --name, --tag etc. use exact/substring matching where all results
507+
// have equivalent relevance.
508+
// If the user explicitly configured output.sortOrder, respect it even
509+
// with queries — that is an explicit user preference.
510+
return;
511+
}
512+
513+
// No settings configured and no query — apply default sort by name
514+
// so that output is deterministic and user-friendly.
515515
sortFields.emplace_back(SortField::Name);
516516
}
517517
}
518518

519-
// Empty sort fields or relevance-only means no sorting
520-
if (sortFields.empty() ||
521-
(sortFields.size() == 1 && sortFields[0] == SortField::Relevance))
519+
// Relevance-only means preserve source ordering — no sorting needed.
520+
if (sortFields.size() == 1 && sortFields[0] == SortField::Relevance)
522521
{
523522
return;
524523
}
@@ -544,9 +543,12 @@ namespace AppInstaller::CLI::Workflow
544543
[mask](const InstalledPackagesTableLine& line, size_t index) {
545544
return SortablePackageEntry(
546545
index,
547-
line.Name.get(), line.Id.get(),
548-
line.InstalledVersion.get(), line.AvailableVersion.get(),
549-
line.Source.get(), mask);
546+
line.Name.get(),
547+
line.Id.get(),
548+
line.InstalledVersion.get(),
549+
line.AvailableVersion.get(),
550+
line.Source.get(),
551+
mask);
550552
},
551553
sortFields, direction);
552554
}

src/AppInstallerCLITests/AppInstallerCLITests.vcxproj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@
302302
<ClCompile Include="TestConfiguration.cpp" />
303303
<ClCompile Include="TestRestRequestHandler.cpp" />
304304
<ClCompile Include="TestSettings.cpp" />
305-
<ClCompile Include="ListSort.cpp" />
305+
<ClCompile Include="PackageTableSortHelper.cpp" />
306306
<ClCompile Include="TestSource.cpp" />
307307
<ClCompile Include="UninstallFlow.cpp" />
308308
<ClCompile Include="UpdateFlow.cpp" />

src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@
236236
<ClCompile Include="TestRestRequestHandler.cpp">
237237
<Filter>Source Files</Filter>
238238
</ClCompile>
239-
<ClCompile Include="ListSort.cpp">
239+
<ClCompile Include="PackageTableSortHelper.cpp">
240240
<Filter>Source Files\CLI</Filter>
241241
</ClCompile>
242242
<ClCompile Include="UserSettings.cpp">

src/AppInstallerCLITests/Command.cpp

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@
44
#include "TestCommon.h"
55
#include <Command.h>
66
#include <AppInstallerStrings.h>
7+
#include <Commands/ListCommand.h>
78
#include <Commands/RootCommand.h>
9+
#include <winget/UserSettings.h>
810

911
using namespace std::string_literals;
1012
using namespace std::string_view_literals;
@@ -665,3 +667,55 @@ TEST_CASE("ParseArguments_PositionalWithTooManyValues", "[command]")
665667

666668
REQUIRE_COMMAND_EXCEPTION(command.ParseArguments(inv, args), CLI::Resource::String::ExtraPositionalError(Utility::LocIndView{ values.back() }));
667669
}
670+
671+
TEST_CASE("EnsureListSortFieldCountMatchesLimit", "[command]")
672+
{
673+
// Verifies that the ListCommand --sort argument count limit matches
674+
// the number of valid SortField enum values. If a new SortField is
675+
// added but SetCountLimit is not updated (or vice versa), this fails.
676+
ListCommand listCmd("");
677+
auto args = listCmd.GetArguments();
678+
679+
size_t sortLimit = 0;
680+
for (const auto& arg : args)
681+
{
682+
if (arg.ExecArgType() == Execution::Args::Type::Sort)
683+
{
684+
sortLimit = arg.Limit();
685+
break;
686+
}
687+
}
688+
REQUIRE(sortLimit > 0);
689+
690+
// Every valid sort field string must convert successfully.
691+
std::vector<std::string_view> allFieldNames = {
692+
"relevance", "name", "id", "version", "source", "available"
693+
};
694+
695+
REQUIRE(allFieldNames.size() == sortLimit);
696+
697+
for (const auto& name : allFieldNames)
698+
{
699+
auto field = Settings::ConvertToSortField(name);
700+
REQUIRE(field.has_value());
701+
}
702+
}
703+
704+
TEST_CASE("ValidateArguments_StandardArgExceedsCountLimit", "[command]")
705+
{
706+
Args args;
707+
TestCommand command({
708+
Argument{ "sort", 's', Args::Type::Sort, DefaultDesc, ArgumentType::Standard }.SetCountLimit(6),
709+
});
710+
711+
// All 6 valid sort fields plus a duplicate — 7 total exceeds the limit of 6.
712+
Invocation inv{ std::vector<std::string>{
713+
"--sort", "name", "--sort", "id", "--sort", "version",
714+
"--sort", "source", "--sort", "available", "--sort", "relevance",
715+
"--sort", "name" } };
716+
717+
command.ParseArguments(inv, args);
718+
REQUIRE(args.GetCount(Args::Type::Sort) == 7);
719+
720+
REQUIRE_COMMAND_EXCEPTION(command.ValidateArguments(args), CLI::Resource::String::TooManyArgError(ArgumentCommon::ForType(Args::Type::Sort).Name));
721+
}
File renamed without changes.

0 commit comments

Comments
 (0)