Default values for required and error css classes
51 views
Skip to first unread message
smi...@gmail.com
Oct 26, 2021, 11:51:27 AM10/26/21
to Django developers (Contributions to Django itself)
Hi all,
I was looking at the code for `forms.Form` recently and noticed that there are a couple of hooks[1] to add CSS classes to required or erroneous forms.
However, the `BaseForm` does not have a class level initialisation of these. The classes are then later added via a `hasattr` check.
I opened this as a ticket [2] and it was triaged as a duplicate of a ticket from 11 years ago [3]. At that time it was closed due to it not considered 'worth setting' along with concerns about backward compatibility. Going back a bit further in time the proposed patch did include placeholders (as `None`) but for reasons I can't quite tell they we're not included in the merged version of the patch.
Significant time has passed since the decision was made and I'm interested in what _best practice_ is _these days_.
I think Carlton nicely summarised the spectrum here on one hand "auto-complete and type checkers much prefer attributes to be defined (as None?) rather than injected later" on the other there is a need to avoid "litter class definitions and/or init methods with None declaration (Better the tooling gets cleverer.)".
I've created a sample patch[5] and shown the extra help a modern IDE (Pycharm in this case) can provide if the variable is initalised as `None` at the class level. I also like that
Appreciate thoughts on this, if only to help me understand API design better!
Kind Regards
David
1 https://docs.djangoproject.com/en/3.2/ref/forms/api/#styling-required-or-erroneous-form-rows
2 https://code.djangoproject.com/ticket/33226
3 https://code.djangoproject.com/ticket/14322
4 https://code.djangoproject.com/ticket/3512#comment:30
5 https://gist.github.com/smithdc1/65605c52f05bd39bf43fed67962b7ece
I was looking at the code for `forms.Form` recently and noticed that there are a couple of hooks[1] to add CSS classes to required or erroneous forms.
However, the `BaseForm` does not have a class level initialisation of these. The classes are then later added via a `hasattr` check.
I opened this as a ticket [2] and it was triaged as a duplicate of a ticket from 11 years ago [3]. At that time it was closed due to it not considered 'worth setting' along with concerns about backward compatibility. Going back a bit further in time the proposed patch did include placeholders (as `None`) but for reasons I can't quite tell they we're not included in the merged version of the patch.
Significant time has passed since the decision was made and I'm interested in what _best practice_ is _these days_.
I think Carlton nicely summarised the spectrum here on one hand "auto-complete and type checkers much prefer attributes to be defined (as None?) rather than injected later" on the other there is a need to avoid "litter class definitions and/or init methods with None declaration (Better the tooling gets cleverer.)".
I've created a sample patch[5] and shown the extra help a modern IDE (Pycharm in this case) can provide if the variable is initalised as `None` at the class level. I also like that
`hasattr(self.form, 'required_css_class')` can become `self.form.required_css_class`. This change passes the test suite, so I'm unsure
what the previous concerns about backwards compatibility are, likely I'm missing something.
So ultimatly I think the question is that if after all this time could we reopen ticket #14322 [3]
Kind Regards
David
1 https://docs.djangoproject.com/en/3.2/ref/forms/api/#styling-required-or-erroneous-form-rows
2 https://code.djangoproject.com/ticket/33226
3 https://code.djangoproject.com/ticket/14322
4 https://code.djangoproject.com/ticket/3512#comment:30
5 https://gist.github.com/smithdc1/65605c52f05bd39bf43fed67962b7ece
Reply all
Reply to author
Forward
0 new messages