Cleanup command descriptions

[bensmrs]

Is there an existing issue for this?

• There is no existing issue for this bug

Is this happening on an up to date version of Incus?

• This is happening on a supported version of Incus

Incus system details

Irrelevant

Instance details

No response

Instance log

No response

Current behavior

While translating command descriptions, I noticed that their descriptions are very irregular, and some elements are either inconsistent or just wrong:

• Some descriptions (both short and long) end with a period and some don’t;
• Some long descriptions are the same as the short ones, but with one or two words rephrased (which feels useless), sometimes with a singular/plural mismatch;
• Some commands suggest that long column format specifiers are accepted while it’s not the case from the source code;
• Some commands indicate “[…] that control which XXX attributes to output […]” with “XXX” being the wrong word(s); this happens at other places in some command descriptions;
• Some filter descriptions suggest that matches are performed on substrings while they sometimes are performed on prefixes only;
• Some filter descriptions indicate that filters should be combined with commas, where they can actually be different command-line arguments (I think comma separation is legacy and shouldn’t even be documented anymore);
• The default column layout isn’t always shown at a consistent place;
• Some of these default column layout characters don’t match the actual allowed characters;
• Some list commands feature a == Columns == separator (which by the way renders poorly on the web documentation) while some don’t.

Expected behavior

(Marking this as a bug because it’s not just a matter of style and taste)

The above comments should be fixed. It’s rather painful to do but will make command descriptions much clearer. If the person tackling this happens to speak French, I’m suggesting them to have a look at what’s being done in the French translation.

I think some of these elements (such as column layout info) should be automatically generated from data present in the code. It would also have the nice effect for translators not to die of boredom when they reach the incus <xxx> list part of the translation :)

Also, do we still need all these line breaks? Content and disposition should be handled at different places if we really want to have fixed-width lines.

Steps to reproduce

/

[bensmrs]

I think I’ll start the grunt work next week. Doesn’t seem fair to just drop the bomb and do nothing about it :)

[bensmrs]

Following #2945, I’ve had a look at automated parsing and my first tests are promising, but this getConfig is really bad parser-wise. I will have to implement an override mechanism under the hood to display <key>=<value> even though the actual parsing rule is (<key>=<value>|<key> <value>)…
I guess dropping the old behavior is out of the question, right?

[stgraber]

Yeah, unfortunately I suspect people's script are a good mix of both syntax...

[bensmrs]

Ok I have a nice solution for this. Now, I have found another implicit parsing rule there:

incus/cmd/incus/cluster.go

Lines 292 to 294 in 7c1eb23

	if resource.name != "" {
	filters = append(filters, resource.name)
	}

This means that we can both do incus cluster list foo: bar baz and incus cluster list foo:bar baz. I find this one pretty unnecessary, but are we reaching for 100% backwards compatibility for such an exotic way of writing filters? That would honestly make the parsing rule quite bulky, and multiplying the amount of non-publicly-advertised argument formats is not the easiest to maintain.

[stgraber]

For this one, I don't mind aligning with the usage and not supporting the other syntax.
That may cause a couple of breakages for some folks out there, but the syntax was never advertised as working.

[bensmrs]

I found a bug in incus warning delete: --all is not handled despite being advertised. --all has very special semantics that change the way the CLI arguments are parsed, so I’m patching my parser accordingly. This is just to let you know that if someone opens an issue, I’m already working on it in the next CLI mega-PR (which I REALLY hope to finish tomorrow). That applies to other commands that may be missing special handling for --all).

[bensmrs]

This is getting a bit out of hand :)
I’m finding a lot of small mistakes and imprecisions, and there are plenty of parsing ambiguities that are hidden under the rug, so I’m fighting a bit against some design choices…
Storage volumes and their bunch of slashes are really painful to handle.

[bensmrs]

Ok, it looks like plenty of storage volume commands support prefixing custom/ (and only custom/) to some arguments (attach, attach-profile, create, delete, move, rename, file mount, and that’s just those I’ve had the time to review yet). That special handling is a direct consequence of using parseVolume, and is not advertised. Shall I follow the usage lines strictly and disallow it? Shall I silently allow it (by adding a parsing rule not shown to the user)? Shall I advertise it?

I’m really starting to lose my sanity on this future PR :D

[stgraber]

I'd say whatever is easiest for you on those ones.

I'd be surprised if anyone actively passed custom/ when it's the default prefix and for most of those commands (show/edit notwithstanding) is the only type allowed anyway.

[bensmrs]

There’s the ease of implementation, but also the consistency between what’s shown to the user and what’s actually accepted by the command, keeping in mind that we can’t have 100+-character long usage strings, else nobody’s going to use those. I’d really like the key/value processing to be the only one with a special logic.