Kord-Extensions/kord-extensions

Arguments should be ordered properly

DRSchlaubi opened this issue · 9 comments

I have a problem that when I have an argument system like this

class SuperArguments : Arguments()
  val x = oprionalInt(...)
}

class MyArgs : SuperArguments() {
  val y = int(...)

Then x will be over y in Arguments.args which Discord doesn't like

One possible way of solving this would be to just put all required args before all optional args, that way we sorta retain the order but still register them like discord wants us to

Another way would be to add an index to the argument builders

Because of how class instantiation order works, I don't think doing anything that relies on the order of instantiating the classes makes sense. However, reordering the declarations is also not going to happen - arguments are always parsed in precisely the order they're defined, which is something that's absolutely required for some use-cases.

The problem here is that there's really no good solution - we can't provide a different way of adding the arguments from the subtype because there'd be no access to its properties, and we can't change the order of the arguments as provided by the user. I don't think there's a realistic way to do this in Kotlin.

However, reordering the declarations is also not going to happen - arguments are always parsed in precisely the order they're defined, which is something that's absolutely required for some use-cases.

That makes no sense whatsoever, re-ordering like this would not affect anything, if the arguments are already ordered correctly, nothing would happen, if it's not the order is broken anyway, so there is no reason to preserve it, as you can't do anything with it.

But if you want to insist, that this order is hyper important, you can simply store arguments as an IndexedValue and then order by that, it would cause some problems with not duplicating indices, but I think those can be fixed

That makes no sense whatsoever, re-ordering like this would not affect anything, if the arguments are already ordered correctly, nothing would happen, if it's not the order is broken anyway, so there is no reason to preserve it, as you can't do anything with it.

val one by string { ... }
val two by somethingElse { 
    ....

    validate {
        failIf(one == ...)
    }
}

A logical API is important, nobody is omnipotent - esp. not you or I.

But if you want to insist, that this order is hyper important, you can simply store arguments as an IndexedValue and then order by that, it would cause some problems with not duplicating indices, but I think those can be fixed

Wouldn't that have the same problem?

That makes no sense whatsoever, re-ordering like this would not affect anything, if the arguments are already ordered correctly, nothing would happen, if it's not the order is broken anyway, so there is no reason to preserve it, as you can't do anything with it.

val one by string { ... }
val two by somethingElse { 
    ....

    validate {
        failIf(one == ...)
    }
}

A logical API is important, nobody is omnipotent - esp. not you or I.

And how does ordering break this API? If the order matches Discord requirements, it is the same order.

If it's not, we use the partition function to determine optional and non-optional arguments and simply move the required ones before the optional ones, so it only happens if it is really needed

Or simply add val autoSort: Boolean = false to the Arguments class and then only do exactly that if some class sets it to true, then nothing would break, except if you explicitly tell it to break

so it only happens if it is really needed

This is an unexpected behaviour for an otherwise straightforward and predictable API. Hidden gotchas like that are definitely not wise to include, IMO.

so it only happens if it is really needed

This is an unexpected behaviour for an otherwise straightforward and predictable API. Hidden gotchas like that are definitely not wise to include, IMO.

I don't see anything unpredictable making it opt-in only

Even then it's still kinda unpredictable, you're just allowed to turn the unpredictable feature on or off

Even then it's still kinda unpredictable, you're just allowed to turn the unpredictable feature on or off

That imo would be a worthy trade-off to work around this Discord restriction, if the functionality is properly documented