Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stubs for WTForms #10557

Merged
merged 11 commits into from
Sep 29, 2023
Merged

Add stubs for WTForms #10557

merged 11 commits into from
Sep 29, 2023

Conversation

Daverball
Copy link
Contributor

This one's a bit of a doozy. I had already attempted to create stubs for this a few years back and gave up because they ended up being too annoying to use since the API isn't very friendly to static analysis.

But I've recently seen some examples in types-PyYAML that gave me renewed hope to attempt this and ended up with something I'm pretty happy with. It's not perfect, there are some things about the API that flat out don't work in type checkers:

Field.__new__ will return by default an UnboundField which is not a subclass of Field and even if I were to make it a faux-subclass and added overloads in __new__ to make it work for the base class, I would then also need to add an overloaded __new__ to every subclass and it would also break user-defined subclasses of Field. So that's not really sustainable. What I did instead was turn Field into a faux descriptor, that will return an UnboundField when accessed from the Form class and itself when it's accessed on the instance, that should cover most use-cases and it required only one additional hacky workaround for FieldList which has to accept Field in addition to UnboundField, if we don't want to force people to manually construct UnboundField instances for type checking.

Other than that I've added a few test cases for the more complex type protocols that can be passed into Field/Form such as validators, widget and filters since it was a little bit tricky to ensure there would be no false negatives while still retaining some amount of type safety.

I've integrated the stubs in two medium sized projects which both do some advanced things with the internals of WTForms in different ways and managed to catch a lot of the rough edges already.

A slight pain point right now is that form validation cannot result in any kind of type narrowing, so you will need to manually add some assert form.field.data is not None for fields with InputRequired/DataRequired validators. It shouldn't be too big of an issue though, since in a lot of cases you should just be using form.populate_obj() rather than accessing individual field data.

There also seems to be an issue with overriding Field.widget in subclasses, since it is both a ClassVar and instance variable. For some reason mypy will complain if subclasses annotate the field as _Widget[Self], even though that matches the definition on the base class, so that seems like a false positive on mypy's end to me. This should only really be relevant though for user defined fields with a custom default widget that have subclasses themselves.

The docs for WTForms are pretty good and can be found here: https://wtforms.readthedocs.io/en/3.0.x/
Although it might still be quicker to just look at the source code: https://github.com/wtforms/wtforms/blob/3.0.1/src/wtforms/__init__.py

@AlexWaygood
Copy link
Member

AlexWaygood commented Aug 10, 2023

To fix the stub-uploader CI failure, you'll just need to file a PR to add MarkupSafe to this set here: https://github.com/typeshed-internal/stub_uploader/blob/ea82cd6fbc62885a8e734dd0b8d113736500a4fc/stub_uploader/metadata.py#L170-L184

(The allowlist has to be in a separate repo for security reasons)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Daverball
Copy link
Contributor Author

Daverball commented Aug 16, 2023

I tried out the stubs in a larger project and it was definitely a little bit more painful to use there, but this can mostly be attributed to the additional abstraction layer that project uses to dynamically create forms and the heavy use of FormField rather than the usability of the stubs.

Other than that I found a small typing issue in wtforms.form.BaseForm.errors due to FormField violating the Liskov substitution principle. I considered changing it to dict[str, Any] to make it less annoying to use, but it doesn't actually come up that often, so I'd rather be more type safe. It's more common that errors will be inspected through individual fields.

There is still a type safety issue with Field.errors on the base class, due to the Liskov violation in FormField but it's probably still preferable over changing Field.errors to Sequence[str] | dict[str, Sequence[str], since otherwise all the subclasses of Field will need to explicitly override errors with the correct type, which is fine for the fields wtforms provides itself, but would be annoying/error prone for any user defined fields that derive from Field.

@github-actions

This comment has been minimized.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks like it was a heroic effort! I read through all the stubs and noted some minor issues.

stubs/WTForms/wtforms/fields/choices.pyi Outdated Show resolved Hide resolved
stubs/WTForms/wtforms/fields/core.pyi Show resolved Hide resolved
stubs/WTForms/wtforms/fields/core.pyi Outdated Show resolved Hide resolved
stubs/WTForms/wtforms/fields/datetime.pyi Show resolved Hide resolved
stubs/WTForms/wtforms/fields/list.pyi Outdated Show resolved Hide resolved
stubs/WTForms/wtforms/fields/simple.pyi Show resolved Hide resolved
stubs/WTForms/wtforms/i18n.pyi Show resolved Hide resolved
stubs/WTForms/wtforms/validators.pyi Outdated Show resolved Hide resolved
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

The mypy-primer hits suggest the | None on the data fields will be annoying for real usage. Should we perhaps use | Any instead, or is there real unsafety in code like the pegen code flagged by mypy-primer? (https://github.com/we-like-parsers/pegen/blob/88e42cf776baa5c9fa1bf8e77993da1fbbf30409/src/pegen/web.py#L51 for reference)

@Daverball
Copy link
Contributor Author

Daverball commented Sep 23, 2023

The mypy-primer hits suggest the | None on the data fields will be annoying for real usage. Should we perhaps use | Any instead, or is there real unsafety in code like the pegen code flagged by mypy-primer? (https://github.com/we-like-parsers/pegen/blob/88e42cf776baa5c9fa1bf8e77993da1fbbf30409/src/pegen/web.py#L51 for reference)

It can be a little bit annoying and there isn't a real unsafety in that particular piece of code, but I ultimately decided that type safety is still worth the little bit of extra hassle, especially considering it doesn't come up if you use populate_obj and you don't really statically know in a type checking context if validate has returned True when you are checking data, so even if the field is DataRequired you don't know for sure that data will not be | None, since that only holds true for successfully validated fields. With InputRequired you have even less of a guarantee, since the field might coerce certain inputs to None and just wants it to be an explicit input value from the submitted form. Imagine for example a three value radio field where it can either be present/absent/unknown, True False None can be an adequate representation of that, but you may wish to force users to click on one of the values, hence InputRequired but the data can still be None.

Most validators should guard against None anyways, since you can't rely on everyone putting the Optional/InputRequired/DataRequired validator first. So it only really comes up for small forms like login/search forms where you directly access the value of one or two fields, I don't think it's worth giving up type safety for that.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pegen (https://github.com/we-like-parsers/pegen)
+ src/pegen/web.py:6: error: Unused "type: ignore" comment  [unused-ignore]
+ src/pegen/web.py:7: error: Unused "type: ignore" comment  [unused-ignore]
+ src/pegen/web.py:55: error: Argument 1 to "make_parser" has incompatible type "str | None"; expected "str"  [arg-type]
+ src/pegen/web.py:56: error: Argument 1 to "parse_string" has incompatible type "str | None"; expected "str"  [arg-type]

@JelleZijlstra JelleZijlstra merged commit f8a673f into python:main Sep 29, 2023
47 checks passed
@Daverball Daverball deleted the wtforms branch September 29, 2023 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants