SelectField backward compatibility and minor QoL improvements

[azmeuk]

I addressed most of the points raised in #922

• Add a post_process step which happens right after process
• Add optional form and field args to SelectField and DataList choices callbacks. Callbacks are resolved during the post_process step, so it can access the other fields .data
• Choice inherit from NamedTuple instead of Dataclass. An additional private _Choice class is needed to pass default values to the tuple members since NamedTuple forbids overriding new
• Restore has_groups and iter_groups
• Backward compatibility for iter_choices returning tuples, for render_options taking value, label, selected instead of choice.
• Support choices as dict as suggested in Support dict for SelectForm.choices #886

The compatibility layer makes the code bigger and harder to read, but hopefully things goes better when we delete the deprecations in 3.4/4.0.

I have tested those modification in downstream projects (wtf-peewee, wtforms-alchemy, starlette-wtf, flask-appbuilder) and it solves all the issues related to SelectField.

/cc @Daverball

[ElLorans]

Couldn't double check everything, but if we were extending a Field now we have to change from iter_choices to _iter_choices_normalized?

[Daverball]

Thanks for working on this, this looks pretty good and alleviates my concerns. I will try to take another deep dive later, to see if there is anything else you or I missed.

I did spot one small issue with the new callback, but that's an easy fix, it just adds a little bit more code.

[azmeuk]

Couldn't double check everything, but if we were extending a Field now we have to change from iter_choices to _iter_choices_normalized?

No, you keep overriding iter_choices. _iter_choices_normalized is just a private helper that normalizes the iter_choices output to keep backward compatibility (and raise deprecation warnings).

[Daverball]

Upon closer review I found a few additional subtle issues, but other than that I think we're good.

[Daverball]

I stand corrected, found a couple more things in the widget code.

[azmeuk]

I think I adressed most of your feedback. I went back to dataclasses with iter for the tuple compatibility, because this is hard to handle default values and inheritance with them. I tested on several dowstream projects and the unit test suites pass.

[Daverball]

I think I adressed most of your feedback. I went back to dataclasses with iter for the tuple compatibility, because this is hard to handle default values and inheritance with them. I tested on several dowstream projects and the unit test suites pass.

Thanks so much, I'll take another closer look later, but there's one thing I can already say now.

Using dataclasses for SelectChoice/DataListChoice seems fine to me, however for Choice I still think a NamedTuple is the way to go. Choice doesn't need any defaults, you can just make all four components mandatory, since it's for internal use only, so you also don't need to use any subclassing tricks, you can just directly use a plain NamedTuple.

class Choice(NamedTuple):
    """
    A rendered option yielded by
    :meth:`SelectFieldBase.iter_choices` and
    :meth:`SelectFieldBase.iter_groups`.

    ``selected`` is computed against the field's current data. To
    declare options on a :class:`SelectField`, use
    :class:`SelectChoice` instead.

    :param value:
        The value that will be sent in the request.
    :param label:
        The label of the option.
    :param selected:
        Whether the option is currently selected. Set by ``iter_choices``;
        you rarely set this yourself.
    :param render_kw:
        A dict containing HTML attributes that will be rendered
        with the option.
    """

    value: str
    label: str
    selected: bool
    render_kw: dict

The main reason to use a NamedTuple for Choice is so that type checkers better understand them as structured types for the purposes of unpacking, you can't unpack a dataclass that has mixed field types and have type checkers know the type of each component. This preserves exact typing information and ergonomics for people that decide to keep their implementations the same, instead of switching to named attribute access, which can make the code longer.

I.e. compare

for value, label, selected, render_kw in field.iter_choices():
    do_something_with(value, label, selected)

to

for choice in field.iter_choices():
    do_something_with(choice.value, choice.label, choice.selected)

Neither code is obviously better than the other. Forcing people to switch their code to the latter, just to preserve good type checking experience seems like unnecessary downstream churn for no real benefit to anyone, when a NamedTuple would've worked just as well. It also avoids any potential risks for breakage in downstream code that expects a tuple, since they will still get one, since NamedTuple classes are subclasses of tuple.

[azmeuk]

Honestly, about named tuples, I think this is good enough like that.

• Expecting people to change their code sounds acceptable for a minor version. That's what people need to do to get rid of deprecation warnings anyway.
• Sooner or later we will have native type annotations Add type annotations #618, and that compatibility layer is already complex enough. I am not sur it worth making design decisions to keep the DX unchanged in the meantime (as long as backward compatibility is maintained)
• The major dowstream projects don't break with this PR

[Daverball]

It's not as much about breaking code in an obvious way, since you've increased runtime compatibility by implementing __iter__ on Choice, but it will degrade what type checkers know about old code, that's making use of unpacking, so it's easier for regressions to sneak in, in the future, if the shape of Choice changes again. I also generally believe that API changes should be made extremely carefully and conservatively, especially on a minor version, that is especially true for any objects returned by your API. Any change that requires or strongly incentivizes downstream users to change their code immediately, needs to be justified extremely well, especially since people generally expect semantic version principles to be applied, so they may pin their dependencies to the next major version. If that contract gets violated it can very easily cause broken older releases, since they're not pinned to the last version they're compatible with.

Since SelectChoice is an entirely new way to define choices on the input side, it's fine to go with whatever layout you prefer, especially since people can wait with migrating to the new API until the next major/minor release and do the upgrade gradually, one module at a time, without needing to do it all at once. Encountering a SelectChoice object will also generally not come as a surprise, since you would've deliberately had to create them first, before you'd see them downstream.

The same is not true on the consumer side of iter_choices and iter_groups, if people suddenly receive objects of a completely different type, they will not get a deprecation warning, unless they specifically subclassed the Select widget and override render_option, they will either get a crash or worse some subtle regression, so I think it's important to strive for maximum compatibility here, which includes type checker compatibility, not just runtime compatibility, since they may very well be stuck on older versions of type stubs for a while, since larger API changes tend to take longer to arrive in typeshed, since they take longer to review. So those type stubs should remain correct for minor version bumps, as long as you don't start using the new API.

There's a very clear difference between the two types of changes, one is acceptable for a minor version, since it can be made gradually and at one's leisure. The other may very well require immediate action, not breaking the biggest downstream open source project test suites is not a good enough argument, especially when it's easy for you to mitigate the risks completely by switching to NamedTuple. Choosing a dataclass over a NamedTuple is taking an unnecessary risk.

TLDR: I personally do not see enough benefits to making Choice a dataclass, over a NamedTuple to justify the decision, so if NamedTuple provides superior backwards compatibility and type checking experience by giving people the freedom to continue unpacking choices instead of accessing attributes by name, it's just a total no brainer to me and I would once again very strongly urge you to change it. I've worked very hard to make types-WTForms a good experience with the proper balance between type coverage and user ergonomics and I'd hate for that experience to degrade, one subtle change at a time, in the interim as we transition to a natively typed API.

[azmeuk]

Thank you for taking time to develop on this point.
I restored named tuples for Choice, and kept dataclasses for DataListChoice and SelectChoice. In the end this did not make as much difference as I expected, since we don't actually need default values for Choice because it is more an internal class.

I let you have a final view on this PR and tell me if you notice additional things.

[Daverball]

Thanks for being so patient and receptive of my feedback. As far as I can tell this addresses any concerns I had and I didn't spot any other potential pitfalls this time around.

I spotted a couple of minor things I'd personally clean up, but they're low priority and won't affect end-users yet either way. So do with them what you want.