[args] Inconsistent order between allowed and allowedHelp

[incendial]

Hello,

Not sure if that was ever discussed before, but allowed respects user defined order of options and allowedHelp for some reason sorts them. This sorting behavior is really inconvenient, and, for example, for numbers results in:

    --rule-version=<all>                           Rule version to include (all by default).

          [1.0.0]                                  Included rules added in 1.0.0.
          [1.1.0]                                  Included rules added in 1.1.0.
          [1.10.0]                                 Included rules added in 1.10.0.
          [1.11.0]                                 Included rules added in 1.11.0.
          [1.2.0]                                  Included rules added in 1.2.0.

which is simply confusing for the end user.

Is there an option to make this behaviour configurable or remove it completely? Considering the inconsistency, it seems like giving the user full control over the order of items makes more sense.

[jakemac53]

These are likely sorted in order to give a stable ordering - it is derived from the keys of the allowedHelp Map, and not all Maps iterate in insertion order (or even necessarily a deterministic order).

We could however change this such that it sorts based on the ordering in allowed, which would probably be the ideal solution?

[jakemac53]

Hmm, it looks like we also support not passing allowed at all.

It also looks like we don't perform any validation that allowed and allowedHelp have any sort of alignment.... but changing that would be breaking.

If you have some items in allowed which are not in allowedHelp, they won't appear in the usage text at all. And any extra entries in allowedHelp will appear.

I also would not be shocked if some people are intentionally adding extra items to allowedHelp which are not in allowed in order to get some special usage text, for instance a single line describing the behavior of many items.

[jakemac53]

Another side note here, if you pass allowedHelp but not allowed, any value is actually allowed. Which also might be useful (maybe you want to document the types of allowed values even though they cant all be enumerated).

[incendial]

These are likely sorted in order to give a stable ordering - it is derived from the keys of the allowedHelp Map, and not all Maps iterate in insertion order (or even necessarily a deterministic order).

Sure, but AFAIK the map literal {} has the insertion-base order. And how often any other map is used in such a case? IMO, giving the user of the package full control over the order would make the most sense.

[jakemac53]

I see several possible ways forward here, please chime in with other ideas as well:

1. Do nothing

   • lowest risk, most other options are technically breaking because they might break existing usage text
   • leaves us in an inconsistent state where you have no control over ordering when using allowedHelp.

2. Just stop sorting (assume most people are using map literals, expect insertion based ordering, and are listing the entries in the order they want them to appear).

   • gives users control
   • might force some weird patterns for the edge cases where users aren't giving map literals (maybe the values are coming from somewhere else, but probably uncommon).
   • could break the usage text for some tools that were relying on the sorting

3. Use ordering from allowed if present, otherwise append to the end of the list using the map keys ordering (or use some other strategy).

   • gives users some control, but it's a bit disconnected
   • can't choose where to put any extra items that don't appear in allowed, without adding extra configuration
   • also breaking for tools that are relying on existing sorting

4. Add some configuration (maybe a compareAllowedHelp or sortAllowedHelp function).

   • most flexible, but also a bit of a pain to use potentially.
   • could be backwards compatible (leave existing behavior by default)
   • adds more API surface area to an already somewhat complex function

[incendial]

I'd add that there is this issue #123 (which is still open) and if you are actually interested in addressing it, maybe introducing some new API will be a solution for both (but this option is probably the worst in terms of effort/potential migration).

Do nothing

I ended up forking args to have a fix, but would prefer to not have a fork.

[jakemac53]

@incendial feel free to chime in with your preferred option and rationale (either from the above or an additional suggestion). I can see an argument for any of the options so any additional thoughts are helpful to guide us in a good direction here.

[incendial]

@incendial feel free to chime in with your preferred option and rationale (either from the above or an additional suggestion). I can see an argument for any of the options so any additional thoughts are helpful to guide us in a good direction here.

If we put potential breaking changes aside, my preferred option would be to just remove the sorting. On the other hand, I have no data on how many packages/devs use this API and whether anyone else prefers or relies on this behaviour and whether the order for them is even relevant (though again, allowed is not sorted/modified anyhow and having consistent behaviour between allowed and allowedHelp would make more sense to me).

So if avoiding breaking changes is preferable, then adding a flag works fine for me. As for the number 3, I think relying on the order of allowed will make the API a lot more confusing than it is now is some cases where the values in allowed and allowedHelp do not fully match.

P.S. Theoretically, the map can be checked if it's a map with a stable order (e.g. LinkedHashMap) and sorted only if it's not, but this will still be a breaking change, unfortunately.

[natebosch]

I think I prefer option 2. The fact that Dart default maps preserve key order is an assumption baked in a fair number of places now, and I think it's idiomatic to treat the key order of Map typed argument as meaningful.

I think it should be safe enough to roll out the technically breaking behavior change as a non-breaking release, but I'd also be OK with a major version bump. I expect we may get some CI failures from hardcoded tests, but I do not expect any usage to be relying on the sorting in a meaningful way. Any use case that was relying on it should be able to construct a map with correctly ordered keys before passing it in.

[jakemac53]

I am comfortable doing option 2 as well and releasing it as non-breaking. The breaking nature is fairly minor, I don't think it could make any actual application crash, just have slightly different usage text.

[jakemac53]

It does bother me more generally how these arguments are handled but that is a different issue and I would rather not conflate the two things :)