Clarification regarding flags

[h-2]

The first argument to add_option ... is the variable to store the value in. Importantly, the previous value of that variable is considered as the default.
What does that mean for flags? As far as I understand it, flags are always false by default and only become true when present on the command line. This means that any previous value of the variable is irrelevant, and it is always assumed to be false.

I would propose the following:

• Add documentation that clarifies this behaviour.
• Check the value of the variable that is provided. If it is true this implies that the user misunderstood the behaviour of flags. in that case, throw an exception that tells the user that flags must be false by default.

[eseiler]

I never noticed that I always use the flags with default false. Makes sense since a flag usually enables something. I can't even come up with a case where you would want the default to be true - maybe for the implementation, but not for the user.

I suggest (patch is for seqan3):

diff --git a/include/seqan3/argument_parser/argument_parser.hpp b/include/seqan3/argument_parser/argument_parser.hpp
index a12071976..3833d3fe1 100644
--- a/include/seqan3/argument_parser/argument_parser.hpp
+++ b/include/seqan3/argument_parser/argument_parser.hpp
@@ -266,6 +266,8 @@ public:
      * \param[in]      long_id   The long identifier for the flag (e.g. "integer").
      * \param[in]      desc      The description of the flag to be shown in the help page.
      * \param[in]      spec      Advanced flag specification, see seqan3::option_spec.
+     *
+     * \throws seqan3::design_error
      */
     void add_flag(bool & value,
                   char const short_id,
@@ -273,6 +275,9 @@ public:
                   std::string const & desc,
                   option_spec const spec = option_spec::standard)
     {
+        if (value)
+            throw design_error{"A flag's default value must be false."};
+
         verify_identifiers(short_id, long_id);
         // copy variables into the lambda because the calls are pushed to a stack
         // and the references would go out of scope.

Needs to be patched both here and in seqan3.

Tasks would be:

• Document behavior of flags (find a good place)
• Apply patch+docs to SeqAn3 (including a unit test)
• Adapt and apply patch+docs to Sharg (including a unit test)

Places for documentation would probably be:

• The documentation of value in the add_flag function where I already added \throws
• Tutorial
• Cookbook - if there is some recipe for flags

Do you concur?

[h-2]

I can't even come up with a case where you would want the default to be true - maybe for the implementation, but not for the user.

Even then, it is not possible with the current implementation. Because if the default is true there is no way to unset the default, adding the flag won't change anything. Note that it is always possible to just "invert" the name of the flag, e.g. --disable-foo.

Do you concur?

LGTM. [Except that it isn't clear to me why the changes need to be done in SeqAn3 and in Sharg, I though SeqAn3 would just import it]

[eseiler]

I can't even come up with a case where you would want the default to be true - maybe for the implementation, but not for the user.

Even then, it is not possible with the current implementation. Because if the default is true there is no way to unset the default, adding the flag won't change anything. Note that it is always possible to just "invert" the name of the flag, e.g. --disable-foo.

We could change it to invert the value of the flag, instead of setting it to true - but like you said, I strongly prefer just inverting the flag name :)

Do you concur?

LGTM. [Except that it isn't clear to me why the changes need to be done in SeqAn3 and in Sharg, I though SeqAn3 would just import it]

It does not, but it will :D

[eseiler]

Should be implemented by #66 and seqan/seqan3#2946