Merge pull request #9167 from obsidiansystems/pre-overhaul-completions

Improve tests and docs prior to refactoring completions

(cherry picked from commit 5442d9b472)
Change-Id: Ief99ac2cd9c92981a9a522d15b9c3daf99182c9d

src/libcmd/command.hh

@@ -34,21 +34,28 @@ struct NixMultiCommand : virtual MultiCommand, virtual Command
 // For the overloaded run methods
 #pragma GCC diagnostic ignored "-Woverloaded-virtual"
 
-/* A command that requires a Nix store. */
+/**
+ * A command that requires a \ref Store "Nix store".
+ */
 struct StoreCommand : virtual Command
 {
     StoreCommand();
     void run() override;
     ref<Store> getStore();
     virtual ref<Store> createStore();
+    /**
+     * Main entry point, with a `Store` provided
+     */
     virtual void run(ref<Store>) = 0;
 
 private:
     std::shared_ptr<Store> _store;
 };
 
-/* A command that copies something between `--from` and `--to`
-   stores. */
+/**
+ * A command that copies something between `--from` and `--to` \ref
+ * Store stores.
+ */
 struct CopyCommand : virtual StoreCommand
 {
     std::string srcUri, dstUri;
@@ -60,6 +67,9 @@ struct CopyCommand : virtual StoreCommand
     ref<Store> getDstStore();
 };
 
+/**
+ * A command that needs to evaluate Nix language expressions.
+ */
 struct EvalCommand : virtual StoreCommand, MixEvalArgs
 {
     bool startReplOnEvalErrors = false;
@@ -79,6 +89,10 @@ private:
     std::shared_ptr<EvalState> evalState;
 };
 
+/**
+ * A mixin class for commands that process flakes, adding a few standard
+ * flake-related options/flags.
+ */
 struct MixFlakeOptions : virtual Args, EvalCommand
 {
     flake::LockFlags lockFlags;
@@ -87,6 +101,14 @@ struct MixFlakeOptions : virtual Args, EvalCommand
 
     MixFlakeOptions();
 
+    /**
+     * The completion for some of these flags depends on the flake(s) in
+     * question.
+     *
+     * This method should be implemented to gather all flakerefs the
+     * command is operating with (presumably specified via some other
+     * arguments) so that the completions for these flags can use them.
+     */
     virtual std::vector<std::string> getFlakesForCompletion()
     { return {}; }
 
@@ -112,15 +134,29 @@ struct SourceExprCommand : virtual Args, MixFlakeOptions
 
     virtual Strings getDefaultFlakeAttrPathPrefixes();
 
+    /**
+     * Complete an installable from the given prefix.
+     */
     void completeInstallable(std::string_view prefix);
 };
 
+/**
+ * A mixin class for commands that need a read-only flag.
+ *
+ * What exactly is "read-only" is unspecified, but it will usually be
+ * the \ref Store "Nix store".
+ */
 struct MixReadOnlyOption : virtual Args
 {
     MixReadOnlyOption();
 };
 
-/* Like InstallablesCommand but the installables are not loaded */
+/**
+ * Like InstallablesCommand but the installables are not loaded.
+ *
+ * This is needed by `CmdRepl` which wants to load (and reload) the
+ * installables itself.
+ */
 struct RawInstallablesCommand : virtual Args, SourceExprCommand
 {
     RawInstallablesCommand();
@@ -129,7 +165,7 @@ struct RawInstallablesCommand : virtual Args, SourceExprCommand
 
     void run(ref<Store> store) override;
 
-    // FIXME make const after CmdRepl's override is fixed up
+    // FIXME make const after `CmdRepl`'s override is fixed up
     virtual void applyDefaultInstallables(std::vector<std::string> & rawInstallables);
 
     bool readFromStdIn = false;
@@ -140,8 +176,11 @@ private:
 
     std::vector<std::string> rawInstallables;
 };
-/* A command that operates on a list of "installables", which can be
-   store paths, attribute paths, Nix expressions, etc. */
+
+/**
+ * A command that operates on a list of "installables", which can be
+ * store paths, attribute paths, Nix expressions, etc.
+ */
 struct InstallablesCommand : RawInstallablesCommand
 {
     virtual void run(ref<Store> store, Installables && installables) = 0;
@@ -149,7 +188,9 @@ struct InstallablesCommand : RawInstallablesCommand
     void run(ref<Store> store, std::vector<std::string> && rawInstallables) override;
 };
 
-/* A command that operates on exactly one "installable" */
+/**
+ * A command that operates on exactly one "installable".
+ */
 struct InstallableCommand : virtual Args, SourceExprCommand
 {
     InstallableCommand();
@@ -175,7 +216,12 @@ struct MixOperateOnOptions : virtual Args
     MixOperateOnOptions();
 };
 
-/* A command that operates on zero or more store paths. */
+/**
+ * A command that operates on zero or more extant store paths.
+ *
+ * If the argument the user passes is a some sort of recipe for a path
+ * not yet built, it must be built first.
+ */
 struct BuiltPathsCommand : InstallablesCommand, virtual MixOperateOnOptions
 {
 private:
@@ -207,7 +253,9 @@ struct StorePathsCommand : public BuiltPathsCommand
     void run(ref<Store> store, BuiltPaths && paths) override;
 };
 
-/* A command that operates on exactly one store path. */
+/**
+ * A command that operates on exactly one store path.
+ */
 struct StorePathCommand : public StorePathsCommand
 {
     virtual void run(ref<Store> store, const StorePath & storePath) = 0;
@@ -215,7 +263,9 @@ struct StorePathCommand : public StorePathsCommand
     void run(ref<Store> store, StorePaths && storePaths) override;
 };
 
-/* A helper class for registering commands globally. */
+/**
+ * A helper class for registering \ref Command commands globally.
+ */
 struct RegisterCommand
 {
     typedef std::map<std::vector<std::string>, std::function<ref<Command>()>> Commands;
@@ -271,7 +321,11 @@ struct MixEnvironment : virtual Args {
 
     MixEnvironment();
 
-    /* Modify global environ based on ignoreEnvironment, keep, and unset. It's expected that exec will be called before this class goes out of scope, otherwise environ will become invalid. */
+    /***
+     * Modify global environ based on `ignoreEnvironment`, `keep`, and
+     * `unset`. It's expected that exec will be called before this class
+     * goes out of scope, otherwise `environ` will become invalid.
+     */
     void setEnviron();
 };
 

src/libutil/args.hh

@@ -39,8 +39,21 @@ public:
 
 protected:
 
+    /**
+     * The largest `size_t` is used to indicate the "any" arity, for
+     * handlers/flags/arguments that accept an arbitrary number of
+     * arguments.
+     */
     static const size_t ArityAny = std::numeric_limits<size_t>::max();
 
+    /**
+     * Arguments (flags/options and positional) have a "handler" which is
+     * caused when the argument is parsed. The handler has an arbitrary side
+     * effect, including possible affect further command-line parsing.
+     *
+     * There are many constructors in order to support many shorthand
+     * initializations, and this is used a lot.
+     */
     struct Handler
     {
         std::function<void(std::vector<std::string>)> fun;
@@ -110,7 +123,12 @@ protected:
         { }
     };
 
-    /* Options. */
+    /**
+     * Description of flags / options
+     *
+     * These are arguments like `-s` or `--long` that can (mostly)
+     * appear in any order.
+     */
     struct Flag
     {
         typedef std::shared_ptr<Flag> ptr;
@@ -130,12 +148,30 @@ protected:
         static Flag mkHashTypeOptFlag(std::string && longName, std::optional<HashType> * oht);
     };
 
+    /**
+     * Index of all registered "long" flag descriptions (flags like
+     * `--long`).
+     */
     std::map<std::string, Flag::ptr> longFlags;
+
+    /**
+     * Index of all registered "short" flag descriptions (flags like
+     * `-s`).
+     */
     std::map<char, Flag::ptr> shortFlags;
 
+    /**
+     * Process a single flag and its arguments, pulling from an iterator
+     * of raw CLI args as needed.
+     */
     virtual bool processFlag(Strings::iterator & pos, Strings::iterator end);
 
-    /* Positional arguments. */
+    /**
+     * Description of positional arguments
+     *
+     * These are arguments that do not start with a `-`, and for which
+     * the order does matter.
+     */
     struct ExpectedArg
     {
         std::string label;
@@ -144,8 +180,24 @@ protected:
         std::function<void(size_t, std::string_view)> completer;
     };
 
+    /**
+     * Queue of expected positional argument forms.
+     *
+     * Positional arugment descriptions are inserted on the back.
+     *
+     * As positional arguments are passed, these are popped from the
+     * front, until there are hopefully none left as all args that were
+     * expected in fact were passed.
+     */
     std::list<ExpectedArg> expectedArgs;
 
+    /**
+     * Process some positional arugments
+     *
+     * @param finish: We have parsed everything else, and these are the only
+     * arguments left. Used because we accumulate some "pending args" we might
+     * have left over.
+     */
     virtual bool processArgs(const Strings & args, bool finish);
 
     virtual Strings::iterator rewriteArgs(Strings & args, Strings::iterator pos)
@@ -204,6 +256,9 @@ public:
 
     friend class MultiCommand;
 
+    /**
+     * The parent command, used if this is a subcommand.
+     */
     MultiCommand * parent = nullptr;
 
 private:

tests/functional/completions.sh

@@ -48,6 +48,8 @@ EOF
 [[ "$(NIX_GET_COMPLETIONS=5 nix build ./foo ./bar --override-input '')" == $'normal\na\t\nb\t' ]]
 ## With tilde expansion
 [[ "$(HOME=$PWD NIX_GET_COMPLETIONS=4 nix build '~/foo' --override-input '')" == $'normal\na\t' ]]
+[[ "$(HOME=$PWD NIX_GET_COMPLETIONS=5 nix flake show '~/foo' --update-input '')" == $'normal\na\t' ]]
+[[ "$(HOME=$PWD NIX_GET_COMPLETIONS=4 nix run '~/foo' --update-input '')" == $'normal\na\t' ]]
 ## Out of order
 [[ "$(NIX_GET_COMPLETIONS=3 nix build --update-input '' ./foo)" == $'normal\na\t' ]]
 [[ "$(NIX_GET_COMPLETIONS=4 nix build ./foo --update-input '' ./bar)" == $'normal\na\t\nb\t' ]]