From 7fff625e39fa6b11c4c61eeacadc70a0253bdab6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= <7226587+thufschmitt@users.noreply.github.com> Date: Wed, 6 Dec 2023 14:13:45 +0100 Subject: [PATCH] =?UTF-8?q?Improve=20the=20error=20message=20for=20?= =?UTF-8?q?=E2=80=9Cmulticommands=E2=80=9D=20commands=20(#9510)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Factor out the default `MultiCommand` behavior All the `MultiCommand`s had (nearly) the same behavior when called without a subcommand. Factor out this behavior into the `NixMultiCommand` class. * Display the list of available subcommands when none is specified Whenever a user runs a command that excepts a subcommand, add the list of available subcommands to the error message. * Print the multi-command lists as Markdown lists This takes more screen real estate, but is also much more readable than a comma-separated list --- src/libcmd/command.cc | 14 ++++++++++++++ src/libcmd/command.hh | 6 +++++- src/libutil/args.cc | 5 +++-- src/libutil/args.hh | 9 ++++++--- src/nix/config.cc | 11 ++--------- src/nix/derivation.cc | 11 ++--------- src/nix/flake.cc | 8 ++++---- src/nix/hash.cc | 11 +++-------- src/nix/main.cc | 2 +- src/nix/nar.cc | 9 +-------- src/nix/profile.cc | 11 +++-------- src/nix/realisation.cc | 11 ++--------- src/nix/registry.cc | 14 ++++---------- src/nix/sigs.cc | 11 +++-------- src/nix/store.cc | 11 ++--------- 15 files changed, 55 insertions(+), 89 deletions(-) diff --git a/src/libcmd/command.cc b/src/libcmd/command.cc index de9f546fc..369fa6004 100644 --- a/src/libcmd/command.cc +++ b/src/libcmd/command.cc @@ -1,4 +1,5 @@ #include "command.hh" +#include "markdown.hh" #include "store-api.hh" #include "local-fs-store.hh" #include "derivations.hh" @@ -34,6 +35,19 @@ nlohmann::json NixMultiCommand::toJSON() return MultiCommand::toJSON(); } +void NixMultiCommand::run() +{ + if (!command) { + std::set subCommandTextLines; + for (auto & [name, _] : commands) + subCommandTextLines.insert(fmt("- `%s`", name)); + std::string markdownError = fmt("`nix %s` requires a sub-command. Available sub-commands:\n\n%s\n", + commandName, concatStringsSep("\n", subCommandTextLines)); + throw UsageError(renderMarkdownToTerminal(markdownError)); + } + command->second->run(); +} + StoreCommand::StoreCommand() { } diff --git a/src/libcmd/command.hh b/src/libcmd/command.hh index 120c832ac..4a72627ed 100644 --- a/src/libcmd/command.hh +++ b/src/libcmd/command.hh @@ -26,9 +26,13 @@ static constexpr Command::Category catNixInstallation = 102; static constexpr auto installablesCategory = "Options that change the interpretation of [installables](@docroot@/command-ref/new-cli/nix.md#installables)"; -struct NixMultiCommand : virtual MultiCommand, virtual Command +struct NixMultiCommand : MultiCommand, virtual Command { nlohmann::json toJSON() override; + + using MultiCommand::MultiCommand; + + virtual void run() override; }; // For the overloaded run methods diff --git a/src/libutil/args.cc b/src/libutil/args.cc index 4480a03f5..c4b2975ee 100644 --- a/src/libutil/args.cc +++ b/src/libutil/args.cc @@ -483,7 +483,7 @@ bool Args::processArgs(const Strings & args, bool finish) if (!anyCompleted) exp.handler.fun(ss); - /* Move the list element to the processedArgs. This is almost the same as + /* Move the list element to the processedArgs. This is almost the same as `processedArgs.push_back(expectedArgs.front()); expectedArgs.pop_front()`, except that it will only adjust the next and prev pointers of the list elements, meaning the actual contents don't move in memory. This is @@ -622,8 +622,9 @@ std::optional Command::experimentalFeature () return { Xp::NixCommand }; } -MultiCommand::MultiCommand(const Commands & commands_) +MultiCommand::MultiCommand(std::string_view commandName, const Commands & commands_) : commands(commands_) + , commandName(commandName) { expectArgs({ .label = "subcommand", diff --git a/src/libutil/args.hh b/src/libutil/args.hh index 7af82b178..72278dccc 100644 --- a/src/libutil/args.hh +++ b/src/libutil/args.hh @@ -223,11 +223,11 @@ protected: std::list expectedArgs; /** * List of processed positional argument forms. - * + * * All items removed from `expectedArgs` are added here. After all * arguments were processed, this list should be exactly the same as * `expectedArgs` was before. - * + * * This list is used to extend the lifetime of the argument forms. * If this is not done, some closures that reference the command * itself will segfault. @@ -356,13 +356,16 @@ public: */ std::optional>> command; - MultiCommand(const Commands & commands); + MultiCommand(std::string_view commandName, const Commands & commands); bool processFlag(Strings::iterator & pos, Strings::iterator end) override; bool processArgs(const Strings & args, bool finish) override; nlohmann::json toJSON() override; + +protected: + std::string commandName = ""; }; Strings argvToStrings(int argc, char * * argv); diff --git a/src/nix/config.cc b/src/nix/config.cc index 5b280d11d..52706afcf 100644 --- a/src/nix/config.cc +++ b/src/nix/config.cc @@ -7,9 +7,9 @@ using namespace nix; -struct CmdConfig : virtual NixMultiCommand +struct CmdConfig : NixMultiCommand { - CmdConfig() : MultiCommand(RegisterCommand::getCommandsFor({"config"})) + CmdConfig() : NixMultiCommand("config", RegisterCommand::getCommandsFor({"config"})) { } std::string description() override @@ -18,13 +18,6 @@ struct CmdConfig : virtual NixMultiCommand } Category category() override { return catUtility; } - - void run() override - { - if (!command) - throw UsageError("'nix config' requires a sub-command."); - command->second->run(); - } }; struct CmdConfigShow : Command, MixJSON diff --git a/src/nix/derivation.cc b/src/nix/derivation.cc index cd3975a4f..59a78d378 100644 --- a/src/nix/derivation.cc +++ b/src/nix/derivation.cc @@ -2,9 +2,9 @@ using namespace nix; -struct CmdDerivation : virtual NixMultiCommand +struct CmdDerivation : NixMultiCommand { - CmdDerivation() : MultiCommand(RegisterCommand::getCommandsFor({"derivation"})) + CmdDerivation() : NixMultiCommand("derivation", RegisterCommand::getCommandsFor({"derivation"})) { } std::string description() override @@ -13,13 +13,6 @@ struct CmdDerivation : virtual NixMultiCommand } Category category() override { return catUtility; } - - void run() override - { - if (!command) - throw UsageError("'nix derivation' requires a sub-command."); - command->second->run(); - } }; static auto rCmdDerivation = registerCommand("derivation"); diff --git a/src/nix/flake.cc b/src/nix/flake.cc index e0c67fdfa..2b6e56283 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -1399,7 +1399,9 @@ struct CmdFlakePrefetch : FlakeCommand, MixJSON struct CmdFlake : NixMultiCommand { CmdFlake() - : MultiCommand({ + : NixMultiCommand( + "flake", + { {"update", []() { return make_ref(); }}, {"lock", []() { return make_ref(); }}, {"metadata", []() { return make_ref(); }}, @@ -1429,10 +1431,8 @@ struct CmdFlake : NixMultiCommand void run() override { - if (!command) - throw UsageError("'nix flake' requires a sub-command."); experimentalFeatureSettings.require(Xp::Flakes); - command->second->run(); + NixMultiCommand::run(); } }; diff --git a/src/nix/hash.cc b/src/nix/hash.cc index d6595dcca..ededf6ef2 100644 --- a/src/nix/hash.cc +++ b/src/nix/hash.cc @@ -130,7 +130,9 @@ struct CmdToBase : Command struct CmdHash : NixMultiCommand { CmdHash() - : MultiCommand({ + : NixMultiCommand( + "hash", + { {"file", []() { return make_ref(FileIngestionMethod::Flat);; }}, {"path", []() { return make_ref(FileIngestionMethod::Recursive); }}, {"to-base16", []() { return make_ref(HashFormat::Base16); }}, @@ -146,13 +148,6 @@ struct CmdHash : NixMultiCommand } Category category() override { return catUtility; } - - void run() override - { - if (!command) - throw UsageError("'nix hash' requires a sub-command."); - command->second->run(); - } }; static auto rCmdHash = registerCommand("hash"); diff --git a/src/nix/main.cc b/src/nix/main.cc index 3d44e4a9d..109d2cc04 100644 --- a/src/nix/main.cc +++ b/src/nix/main.cc @@ -67,7 +67,7 @@ struct NixArgs : virtual MultiCommand, virtual MixCommonArgs, virtual RootArgs bool helpRequested = false; bool showVersion = false; - NixArgs() : MultiCommand(RegisterCommand::getCommandsFor({})), MixCommonArgs("nix") + NixArgs() : MultiCommand("", RegisterCommand::getCommandsFor({})), MixCommonArgs("nix") { categories.clear(); categories[catHelp] = "Help commands"; diff --git a/src/nix/nar.cc b/src/nix/nar.cc index 9815410cf..8ad4f92a7 100644 --- a/src/nix/nar.cc +++ b/src/nix/nar.cc @@ -4,7 +4,7 @@ using namespace nix; struct CmdNar : NixMultiCommand { - CmdNar() : MultiCommand(RegisterCommand::getCommandsFor({"nar"})) + CmdNar() : NixMultiCommand("nar", RegisterCommand::getCommandsFor({"nar"})) { } std::string description() override @@ -20,13 +20,6 @@ struct CmdNar : NixMultiCommand } Category category() override { return catUtility; } - - void run() override - { - if (!command) - throw UsageError("'nix nar' requires a sub-command."); - command->second->run(); - } }; static auto rCmdNar = registerCommand("nar"); diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 476ddcd60..147b4680b 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -825,7 +825,9 @@ struct CmdProfileWipeHistory : virtual StoreCommand, MixDefaultProfile, MixDryRu struct CmdProfile : NixMultiCommand { CmdProfile() - : MultiCommand({ + : NixMultiCommand( + "profile", + { {"install", []() { return make_ref(); }}, {"remove", []() { return make_ref(); }}, {"upgrade", []() { return make_ref(); }}, @@ -848,13 +850,6 @@ struct CmdProfile : NixMultiCommand #include "profile.md" ; } - - void run() override - { - if (!command) - throw UsageError("'nix profile' requires a sub-command."); - command->second->run(); - } }; static auto rCmdProfile = registerCommand("profile"); diff --git a/src/nix/realisation.cc b/src/nix/realisation.cc index e19e93219..e1f231222 100644 --- a/src/nix/realisation.cc +++ b/src/nix/realisation.cc @@ -5,9 +5,9 @@ using namespace nix; -struct CmdRealisation : virtual NixMultiCommand +struct CmdRealisation : NixMultiCommand { - CmdRealisation() : MultiCommand(RegisterCommand::getCommandsFor({"realisation"})) + CmdRealisation() : NixMultiCommand("realisation", RegisterCommand::getCommandsFor({"realisation"})) { } std::string description() override @@ -16,13 +16,6 @@ struct CmdRealisation : virtual NixMultiCommand } Category category() override { return catUtility; } - - void run() override - { - if (!command) - throw UsageError("'nix realisation' requires a sub-command."); - command->second->run(); - } }; static auto rCmdRealisation = registerCommand("realisation"); diff --git a/src/nix/registry.cc b/src/nix/registry.cc index f509ccae8..0346ec1e0 100644 --- a/src/nix/registry.cc +++ b/src/nix/registry.cc @@ -196,10 +196,12 @@ struct CmdRegistryPin : RegistryCommand, EvalCommand } }; -struct CmdRegistry : virtual NixMultiCommand +struct CmdRegistry : NixMultiCommand { CmdRegistry() - : MultiCommand({ + : NixMultiCommand( + "registry", + { {"list", []() { return make_ref(); }}, {"add", []() { return make_ref(); }}, {"remove", []() { return make_ref(); }}, @@ -221,14 +223,6 @@ struct CmdRegistry : virtual NixMultiCommand } Category category() override { return catSecondary; } - - void run() override - { - experimentalFeatureSettings.require(Xp::Flakes); - if (!command) - throw UsageError("'nix registry' requires a sub-command."); - command->second->run(); - } }; static auto rCmdRegistry = registerCommand("registry"); diff --git a/src/nix/sigs.cc b/src/nix/sigs.cc index 39555c9ea..a57a407e6 100644 --- a/src/nix/sigs.cc +++ b/src/nix/sigs.cc @@ -205,7 +205,9 @@ struct CmdKeyConvertSecretToPublic : Command struct CmdKey : NixMultiCommand { CmdKey() - : MultiCommand({ + : NixMultiCommand( + "key", + { {"generate-secret", []() { return make_ref(); }}, {"convert-secret-to-public", []() { return make_ref(); }}, }) @@ -218,13 +220,6 @@ struct CmdKey : NixMultiCommand } Category category() override { return catUtility; } - - void run() override - { - if (!command) - throw UsageError("'nix key' requires a sub-command."); - command->second->run(); - } }; static auto rCmdKey = registerCommand("key"); diff --git a/src/nix/store.cc b/src/nix/store.cc index 2879e03b3..79b41e096 100644 --- a/src/nix/store.cc +++ b/src/nix/store.cc @@ -2,9 +2,9 @@ using namespace nix; -struct CmdStore : virtual NixMultiCommand +struct CmdStore : NixMultiCommand { - CmdStore() : MultiCommand(RegisterCommand::getCommandsFor({"store"})) + CmdStore() : NixMultiCommand("store", RegisterCommand::getCommandsFor({"store"})) { } std::string description() override @@ -13,13 +13,6 @@ struct CmdStore : virtual NixMultiCommand } Category category() override { return catUtility; } - - void run() override - { - if (!command) - throw UsageError("'nix store' requires a sub-command."); - command->second->run(); - } }; static auto rCmdStore = registerCommand("store");