Changeset [5231] -- do_clean_*() is awkward naming
Adrian Holovaty
I just spotted changeset [5231]:
"""
Renamed form-specific cleaning methods to be do_clean_*, rather than clean_*.
This avoids a name clash that would occur when you had a form field called
"data" (because clean_data is already a dictionary on the Form class).
"""
http://code.djangoproject.com/changeset/5231
Well spotted, Malcolm! But I think do_clean_*() is awkward sounding.
Could we think of a better naming convention, ideally one that uses a
single word rather than two ("do clean")?
Some suggestions to get the discussion started:
cleanfield_foo()
validate_foo()
Adrian
--
Adrian Holovaty
holovaty.com | djangoproject.com
Malcolm Tredinnick
Sure, I'm not particularly attached to the name, either. It's entirely
undocumented at the moment, so feel free to make up a better name.
I think we should try to stay with the "cleaning" line of thinking,
rather than validate- or normalise-related terms, just for consistency,
though.
Malcolm
Bruce Kroeze
Max Battcher
> rather than validate- or normalise-related terms, just for consistency,
> though.
Sounds like time to visit Ye Olde Thesaurus... Off the top of my
head: scrub or wash are very useful clean-connected words.
--
--Max Battcher--
http://www.worldmaker.net/
Benjamin Slavin
> rather than validate- or normalise-related terms, just for consistency,
> though.
Agreed.
What about clean_data -> cleaned_data and maintain clean_*()
The biggest down side is that clean_data is already documented... but
it's an easy find-replace
To me, cleanfield_* and do_clean_* seem overly wordy and, as Malcolm
pointed out, the "clean" metaphor is a good one.
- Ben
Malcolm Tredinnick
> On 5/14/07, Malcolm Tredinnick <mal...@pointy-stick.com> wrote:
> > I think we should try to stay with the "cleaning" line of thinking,
> > rather than validate- or normalise-related terms, just for consistency,
> > though.
>
> Agreed.
>
>
> What about clean_data -> cleaned_data and maintain clean_*()
>
> The biggest down side is that clean_data is already documented... but
> it's an easy find-replace
That was exactly why I didn't change clean_data (my initial "fix" was
just to name it cleaned_data). It's already used in lots of code, so the
impact of change it is larger. Changing the undocumented feature is less
disruptive. Unnecessarily intrusive backwards-incompat changes seem
mean.
Malcolm
David Danier
I would go for this. cleaned_data sounds a lot more more adequate anyway.
> The biggest down side is that clean_data is already documented... but
> it's an easy find-replace
As newforms is not fully documented so far and some parts are missing in
0.96 (see bug 3297 for example) some people (including me) used the
source as documentation. So both changes may break things. However
clean_data is the official name for it in 0.96 (including the docs).
> To me, cleanfield_* and do_clean_* seem overly wordy and, as Malcolm
> pointed out, the "clean" metaphor is a good one.
Adding something to "clean" (prefix or suffix) just seems wrong (and
produces more chars). Perhaps some synonym
(http://thesaurus.reference.com/browse/clean) might help? tidy, clear?
Or just add an underscore (_clean_FIELD/clean__FIELD)?
validate_FIELD sounds right, too.
Greetings, David Danier
John Shaffer
For me, an AttributeError from clean_data is a much more friendly
change than silently ignoring code like:
def clean_email(self):
"""Prevent account hijacking by disallowing duplicate emails."""
Adrian Holovaty
> For me, an AttributeError from clean_data is a much more friendly
> change than silently ignoring code like:
>
> def clean_email(self):
> """Prevent account hijacking by disallowing duplicate emails."""
John makes an excellent point. I'm +1 on changing clean_data to
cleaned_data and keeping the "clean_XXX" method hooks as-is.
David Danier
> change than silently ignoring code like:
True, and the AttributeError can even be user-friendly:
class BaseForm(StrAndUnicode):
[...]
_clean_data_error(self):
raise AttributeError("clean_data has been replaced by cleaned_data")
clean_data = property(_clean_data_error)
[...]
Classes extending this can override clean_data with some method
validating a data-field, users get some nice message, full_clean()
writes to cleaned_data and this can be removed in the realese after the
next one.
Greetings, David Danier
Malcolm Tredinnick
> On 5/14/07, John Shaffer <jshaff...@gmail.com> wrote:
> > For me, an AttributeError from clean_data is a much more friendly
> > change than silently ignoring code like:
> >
> > def clean_email(self):
> > """Prevent account hijacking by disallowing duplicate emails."""
>
> John makes an excellent point. I'm +1 on changing clean_data to
> cleaned_data and keeping the "clean_XXX" method hooks as-is.
Everything switched around again in [5237].
Malcolm
Adrian Holovaty
Excellent. Thanks, Malcolm!
Etienne Robillard
I dont understand that changes. For me thats just odd, and
a royal pain to update lots of sites just for renaming clean_data to cleaned_data.
I really think thats odd and a better rationale would take in consideration
that already working and defined functions should try to keep at least some kind
of backward compatible hooks at least for being nice to users of clean_data!
Why not incorporating the semantics or desired logic of cleaned_data into clean_data, so
that at least that part of the newforms api would stay as in the documentation ?
Etienne
Malcolm Tredinnick
>
> I dont understand that changes. For me thats just odd, and
> a royal pain to update lots of sites just for renaming clean_data to cleaned_data.
It's good programming practice that when a backwards incompatible change
is introduced (and sometimes they can't be avoided), potentially broken
code does not keep working silently.
Originally, I didn't follow this practice as well as I could have. John
pointed out where it would be a problem in the exact fragment you
quoted. Nobody came up with a better solution that preserved both
backwards-compatibility for clean_data and non-silent breakage for
existing code relying on the old, no longer present behaviour.
>
> I really think thats odd and a better rationale would take in consideration
> that already working and defined functions should try to keep at least some kind
> of backward compatible hooks at least for being nice to users of clean_data!
That was what we originally tried to do and it would lead to the silent
failure that John pointed out.
We have been quite up front about the fact that there will be backwards
compatible changes to Django leading up to 1.0. They aren't incredibly
common, but they sometimes cannot be helped. If you have code that is
already running on Django and working correctly, that code isn't
affected by these changes because you don't need to update the Django
version it is running against. When you do want/need to update the
underlying Django framework for a particular project, it will be because
you are doing other development work anyway, so a global search and
replace changing clean_data to cleaned_data is only 10 seconds extra
work. We're not breaking backwards compatibility in security updates to
0.95/0.96 or anything like that, after all. By its very design, this
change was designed to break code that hasn't been upgraded, so it's
easy to tell when you have forgotten to do the upgrade because your
tests will fail.
> Why not incorporating the semantics or desired logic of cleaned_data into clean_data, so
> that at least that part of the newforms api would stay as in the documentation ?
The attribute "cleaned_data", which contains the normalised form data is
quite different from the function clean_data() which takes unsanitised
data and validates and normalises it. Combining those to together is not
very good separation of purpose.
Regards,
Malcolm