Constructor argument cleanups

[mhsmith]

Widget constructor arguments follow a fairly consistent order:

• A single required or effectivey-required argument, if any, such as Label(text) and ImageView(image)
• id
• style
• Everything else

Currently all arguments can be passed either with positional or keyword syntax. However, using positional syntax for anything except the required argument is not very clear. So I'd like to propose that we deprecate passing all other arguments positionally.

To detect how an argument was passed, all constructors would need to have a signature of the form:

(required_arg_if_any, *args, **kwargs)

They could then parse the args and kwargs by passing them to a helper function along with a tuple specifying the deprecated positional order. After a reasonable deprecation period, the args part of the signature would be removed.

The docstring of kwargs would change from "initial style properties" to "initial properties", thus making explicit the practice that we've long followed, that every writable property has a constructor argument.

This means the constructor signatures would no longer list all their arguments individually. But I actually think that would be an improvement, as the constructor documentation mostly duplicates the property documentation anyway. The only additional information it contains is the default values, and they can be moved to the documentation of the individual properties.

So far I've only mentioned widgets, but this pattern could apply to other classes as well.

[freakboy3742]

I agree that we should massively discourage the use of positional arguments. If nothing else, it will make our migration path easier, as we don't have to worry about parsing specific argument orders.

However I'm not completely sure I agree with what you're proposing (at least, as I understand it).

For example - it sounds like you're proposing that Label's constructor (minus type annotations) should literally be:

def __init__(self, text, *args, **kwargs):

That's unarguably more generic, but means a loss of detail for any code inspection tools in your IDE (including any type annotation-based checks), and requires that we duplicate a bunch of argument handling logic that Python gives us for free. The benefit you mention of duplication properties is also predicated on the properties being 1-1 matches for their constructor arguments... and I'm not 100% sure that's actually the case (I could be wrong, but it's setting off some bells in my mind). At the very least, there's a couple of places where we have "only available during construction" arguments.

I'd argue that:

def __init__(self, text, *, id, style, **style_kwargs):

would be a better end state - that is:

• reject any positional arguments except the "known required" ones - essentially the text on widgets that must have labels - and even some of those I'd be open to making required positional arguments.
• Make everything else a "must be positional" argument
• use the open ended ** for the "style" arguments.

That means we get all the built-in argument handling and type annotation checking that Python provides; but enforces the use of keyword arguments.

[mhsmith]

That's unarguably more generic, but means a loss of detail for any code inspection tools in your IDE (including any type annotation-based checks)

That's a good point. In that case, let's take Button as an example. The current signature is:

(text=None, icon=None, id=None, style=None, on_press=None, enabled=True, **kwargs)

The transitional signature would be:

(text=None, *args, icon=None, id=None, style=None, on_press=None, enabled=True, **kwargs)

In the transitional phase, the first thing each constructor would do is call a helper function to extract any deprecated positional arguments from *args, raising a warning if they exist. Something like this:

icon, id, style, on_press, enabled = parse_positional(
    args, icon=icon, id=id, style=style, on_press=on_press, enabled=enabled
)

After the deprecation period, the final signature would be:

(text=None, *, icon=None, id=None, style=None, on_press=None, enabled=True, **kwargs)

The benefit you mention of duplication properties is also predicated on the properties being 1-1 matches for their constructor arguments... and I'm not 100% sure that's actually the case (I could be wrong, but it's setting off some bells in my mind). At the very least, there's a couple of places where we have "only available during construction" arguments.

I think even in those cases there's usually a read-only property with the same name. For example, id is such a property, although the fact that it's read-only isn't currently documented.

Anyway, if we change the signature as laid out above, then the duplicate documentation would be an independent issue which could be fixed before, after, or not at all. In most cases, I think it would be clearer for the constructor documentation to cover the initialization-specific details, if any, and then finish with a sentence like "All other arguments are assigned to the properties with the same names."

[freakboy3742]

This makes sense to me as a transition plan and final state.

Anyway, if we change the signature as laid out above, then the duplicate documentation would be an independent issue which could be fixed before, after, or not at all. In most cases, I think it would be clearer for the constructor documentation to cover the initialization-specific details, if any, and then finish with a sentence like "All other arguments are assigned to the properties with the same names."

I agree that at the conceptual level, there's an opportunity for de-duplication of documentation here; I have 2 concerns about documenting the arguments on the constructor as "see properties".

1. Sphinx doesn't make a distinction between properties and methods that reflects the significance of the distinction from the perspective of Toga. As a result, finding the list of properties that a widget supports isn't necessarily trivial - you need to scan the list of all attributes of the Widget class to find the ones that might be properties.
2. IDE hints will be significantly degraded. Take Slider as an example; right now, the constructor hint tells you the defaults for value, min and max; if the docstring is modified to "see docs for the property", that detail will be lost.

However, I also agree that this could be tackled as a follow-up concern.