out_file extensions doesn't work

[h-2]

Does this problem persist on the current master?

• I have verified the issue on the current master

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

    parser.add_positional_option(options.input, "The input file.", seqan3::input_file_validator{{"vcf", "vcf.gz", "bcf"}});
    parser.add_positional_option(options.output, "The output file.", seqan3::output_file_validator{{"vcf", "vcf.gz", "bcf"}});

The first line works, the second does not.

Expected Behavior

The second line should work, too.

P.S: The {{ ... }} is a bit clunky. An operator() with pack would be nice...

Steps To Reproduce

see above

Environment

- Operating system: Ubuntu 21.10
- Sharg version: SeqAn3 master at ffeacc8a294bbcec172a4198910a61b881481935
- Compiler: GCC11.2

Anything else?

Issue template is fancy 🎉

P.S: I didn't try with standalone sharg-master because I didn't see any changes in the respective area.

[eseiler]

#include <seqan3/argument_parser/all.hpp>

struct parser_options
{
    std::filesystem::path input{};
    std::filesystem::path output{};
};

int main(int argc, char ** argv)
{
    parser_options options{};
    
    seqan3::argument_parser parser{"sharg-issue-63", argc, argv};
    parser.add_positional_option(options.input, "The input file.", seqan3::input_file_validator{{"vcf", "vcf.gz", "bcf"}});
    parser.add_positional_option(options.output, "The output file.", seqan3::output_file_validator{{"vcf", "vcf.gz", "bcf"}});
}

gcc-11 log (click me)

<source>: In function ‘int main(int, char**)’:
<source>:15:124: error: class template argument deduction failed:
   15 |     parser.add_positional_option(options.output, "The output file.", seqan3::output_file_validator{{"vcf", "vcf.gz", "bcf"}});
      |                                                                                                                            ^
<source>:15:124: error: no matching function for call to ‘output_file_validator(<brace-enclosed initializer list>)’
In file included from /seqan3/argument_parser/detail/format_base.hpp:25,
                 from /seqan3/argument_parser/detail/format_help.hpp:20,
                 from /seqan3/argument_parser/argument_parser.hpp:25,
                 from /seqan3/argument_parser/all.hpp:43,
                 from <source>:1:
/seqan3/argument_parser/validators.hpp:674:14: note: candidate: ‘template<class file_t> output_file_validator(seqan3::output_file_open_options, std::vector<std::__cxx11::basic_string<char> >)-> seqan3::output_file_validator<file_t>’
  674 |     explicit output_file_validator(output_file_open_options const mode,
      |              ^~~~~~~~~~~~~~~~~~~~~
/seqan3/argument_parser/validators.hpp:674:14: note:   template argument deduction/substitution failed:
<source>:15:124: note:   cannot convert ‘{"vcf", "vcf.gz", "bcf"}’ (type ‘<brace-enclosed initializer list>’) to type ‘seqan3::output_file_open_options’
   15 |     parser.add_positional_option(options.output, "The output file.", seqan3::output_file_validator{{"vcf", "vcf.gz", "bcf"}});
      |                                                                                                                            ^
In file included from /seqan3/argument_parser/detail/format_base.hpp:25,
                 from /seqan3/argument_parser/detail/format_help.hpp:20,
                 from /seqan3/argument_parser/argument_parser.hpp:25,
                 from /seqan3/argument_parser/all.hpp:43,
                 from <source>:1:
/seqan3/argument_parser/validators.hpp:663:5: note: candidate: ‘output_file_validator(seqan3::output_file_validator<file_t>&&)-> seqan3::output_file_validator<file_t> [with file_t = void]’
  663 |     output_file_validator(output_file_validator &&) = default; //!< Defaulted.
      |     ^~~~~~~~~~~~~~~~~~~~~
/seqan3/argument_parser/validators.hpp:663:5: note:   no known conversion for argument 1 from ‘<brace-enclosed initializer list>’ to ‘seqan3::output_file_validator<void>&&’
/seqan3/argument_parser/validators.hpp:662:5: note: candidate: ‘output_file_validator(const seqan3::output_file_validator<file_t>&)-> seqan3::output_file_validator<file_t> [with file_t = void]’
  662 |     output_file_validator(output_file_validator const &) = default; //!< Defaulted.
      |     ^~~~~~~~~~~~~~~~~~~~~
/seqan3/argument_parser/validators.hpp:662:5: note:   no known conversion for argument 1 from ‘<brace-enclosed initializer list>’ to ‘const seqan3::output_file_validator<void>&’
/seqan3/argument_parser/validators.hpp:659:5: note: candidate: ‘template<class file_t> output_file_validator()-> seqan3::output_file_validator<file_t>’
  659 |     output_file_validator() : output_file_validator{output_file_open_options::create_new}
      |     ^~~~~~~~~~~~~~~~~~~~~
/seqan3/argument_parser/validators.hpp:659:5: note:   template argument deduction/substitution failed:
<source>:15:124: note:   candidate expects 0 arguments, 1 provided
   15 |     parser.add_positional_option(options.output, "The output file.", seqan3::output_file_validator{{"vcf", "vcf.gz", "bcf"}});
      |                                                                                                                            ^
In file included from /seqan3/argument_parser/detail/format_base.hpp:25,
                 from /seqan3/argument_parser/detail/format_help.hpp:20,
                 from /seqan3/argument_parser/argument_parser.hpp:25,
                 from /seqan3/argument_parser/all.hpp:43,
                 from <source>:1:
/seqan3/argument_parser/validators.hpp:645:7: note: candidate: ‘output_file_validator(seqan3::output_file_validator<file_t>)-> seqan3::output_file_validator<file_t> [with file_t = void]’
  645 | class output_file_validator : public file_validator_base
      |       ^~~~~~~~~~~~~~~~~~~~~
/seqan3/argument_parser/validators.hpp:645:7: note:   no known conversion for argument 1 from ‘<brace-enclosed initializer list>’ to ‘seqan3::output_file_validator<void>’
Compiler returned: 1

[eseiler]

The seqan::output_file_validator gets a file open mode as first option (doc).

In addition, the validator receives a seqan3::output_file_open_options which allows you to specify what to do if your
output file already exists. seqan3::output_file_open_options::create_new will throw a seqan3::validation_error
exception if it already exists and seqan3::output_file_open_options::open_or_create will skip this check (that means
you are allowed to overwrite the existing file).

As far as I remember, we (not just core, but whole team) discussed having a default, but there are pro and contra arguments for both options. Also, it seems the default in other tools/libs/... is also very mixed.

However, it's a horrible error message, and very hard to see from the compiler output what's going wrong. Especially since you are right to expect the syntax to be the same for in-/ and output - but as the option needs to be set, and extensions have a default, it's not the same.

How could the error be made more clear?
Maybe add a constructor

explicit output_file_validator(std::vector<std::string> extensions)

that just static_asserts?

P.S. I like the idea of making it less chunky {{}}. How would your idea look like in actual usage?
Currently, the extensions are constructor arguments, and operator() is responsible for the validation of a given path. There could be multiple operator(), but I suppose that's also confusing.

[h-2]

Why not just have two constructors?

explicit output_file_validator(auto ... && args) 
    requires ((std::constructible_from<std::string_view, decltype(args)> && ...))
{
    // construct from extensions; if no arguments are provided, use the default extensions
}

explicit output_file_validator(output_file_open_options, auto ... && args) 
    requires ((std::constructible_from<std::string_view, decltype(args)> && ...))
{
    // construct from options + extensions
}

This also gets rid of the {{}}-ugliness.

[eseiler]

We will discuss this in core this week or on Monday.

The general problem seems that we always need a output_file_open_options and we prefer not to have a default for that.
But, more to follow :)

[smehringer]

Core-Meeting 2022-03-15

We will:

• Not add a default for output_file_open_options because there is no sensible default and the user should think about this
• Make the error message better
• Add a constructor for passing the extensions only
• Add constructors that do not need the chunky {{}}

[h-2]

Thank you for discussing this and adding a better way to specify the extensions. I think a default would be nice, as typical I/O (std::ofstream, "w" in std::ostream or python) also allows writing to existing files by default.

I also think that the current names are not very intuitive, in particular create_new sounds like "even if it is there, create a new file". The documentation only mentions "overwriting" in the description, while appending to an existing file is also quite possible.

I would just go for

• ::default "File may or may not exist already."
• ::careful "Throw an exception if the file already exists."

And avoid all talk of "opening", "creating" or "overwriting" because the argument parser does neither of these things, and any of them may or may not happen later on. But it's all not super-important 🙂

[eseiler]

We did already discuss the naming en banc 😁

However, my memory failed me in some other regard:
We do actually use create_new as default (default ctor), but not when also passing a list of possible extensions.
So, it would be sensible to just use the same default if only given a list of extensions.

[smehringer]

Oh, I searched for the wrong keywords when browsing the core meeting notes and didn't find this.

[eseiler]

I opened #71

I think it should address everything :)

[eseiler]

@h-2 One more question:

Would you like us to immediately port the fix to seqan3?

We intend to replace the argparser in seqan3 with sharg once #70 is merged (this PR gets rid of the seqan3 dependency).

[h-2]

Would you like us to immediately port the fix to seqan3?

Nah, no point investing extra work in the SeqAn3 version if it will be replaced soon anyway! But thanks for asking!