BUG: Retry reverse complement flag should not expect arg

[corneliusroemer]

I see the following in CLI help:

--retry-reverse-complement <RETRY_REVERSE_COMPLEMENT>
            Retry seed matching step with a reverse complement if the first attempt failed

The <RETRY_REVERSE_COMPLEMENT> should not be there I think as this is a binary flag - or is this supposed to be true/false? In that case, we should mention that in the help.

EDIT: I tested and indeed, one can add true/false (optionally), and if false, it does not reverse complement.

[corneliusroemer]

I guess there should be brackets around something like [true/false] to indicate possible options?

[ivan-aksamentov]

I think as this is a binary flag - or is this supposed to be true/false

Yes, It's meant to be a binary flag - present or not present, but as you've checked it can also unexpectedly have a value. But I would focus user on that possibility, as all other boolean flags already work without values.

There is some unfortunate architectural shenanigans I had to commit to to make alignment params work okay-ish.

In particular, I am using optfield crate to create a copy of the struct, where all the fields are the same as in the original, but of type Option<T> instead of original T. https://github.com/nextstrain/nextclade/blob/348e65cff85db7d1412a5426189a78c0ddc33b1a/packages_rs/nextclade/src/align/params.rs#L17

This is needed to make sure that all flags are optional, as well as the fields that comes from virus properties file, so that they can be merged with the defaults and produce a non-optional struct for consumption by the algorithm. However, clap handles bool and Option<bool> differently, therefore the observed behavior.

One solution is to have the optional struct defined manually, and then copy the fields over from CLI, virus properties and merge with defaults. However this is additional, highly non-DRY code, and I am 99% certain I forget to also add a copy snippet whenever a new field is added.

Another solution is to use an optional struct in the algorithm, but then alignment code will be polluted by None-checks, and it is unclear what to do in case of None.

If someone knows how to fix that - contributions are welcome.

Right now I don't think anyone will notice that the flag accepts values. For now, can we just say we ignore it? :)

P.S. There may also be a way to just hide the value string from the help message.

[corneliusroemer]

What about adding some extra comments in the help? Like that it optionally accepts false/true? Then it's clear for the user what happens - not perfect but better than having something there with unclear/unexplained function.

[ivan-aksamentov]

Like that it optionally accepts false/true?

I don't see much value on focusing user's attention on that little quirk. You'd need to also add it to all other booleans in this struct. But not outside the struct. And not forget to add it for new booleans. Otherwise it will be asymmetric.

Me personally, I never read these values, they are just noise to me anyways. And I think adding explanations is adding more noise in this case.

But feel free to submit a PR if you feel extra explanations are needed.