Re: newforms issues
On Tue, 2007-05-22 at 21:52 +, SmileyChris wrote: > > #3896 - pass value to field specific clean function [2] > > How expensive is a try/except? > > Call clean_foo passing the value as a parameter and if that fails, use > the old method (setting the value in cleaned_data, then calling > clean_foo with no parameters) > > Seems backwards compatible to me. -1 from me. Either we change the method or we don't. Allowing both is just confusing in cases like this (and, at the risk of sounding like an old man, I've worked with APIs that have tried to do that in the past; I'm speaking from bitter experien). Having old and new methods is workable if you're trying to maintain compatibility, but having both behaviours called the same name is unpleasant. Malcolm --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: newforms issues
>> #3718 - newforms.Form.clean should have access to field errors [1]
Would like to see this one, too. Some validation I do currently is based
on the not-in-cleaned_data-trick, but this is IMHO not very clean.
And may be due to other reasons. For example if the field-clean-method
you want to validate data with is executed before the field is filled
(can happen when some field depends on data of some other field,
example: clean_password_repeat).
> Call me +0.5. I'd like to see this included, but I'll certainly defer
> to Adrian/Jacob on this one - after all, the status quo does work.
But it is not very DRYish, as you have to write the field-name twice:
* In the function-name
* Inside the function as a parameter to access cleaned_data
This may lead to complicated code when having a clean-function that is
used in multiple forms or inside the same form multiple times with
different field-names. Currently I solve this by using this code
(classes with __call__ defined could be used instead, but this makes
things worse):
--8<--
def clean_foo(field_name):
def real_clean_function(form):
# do something with form.cleaned_data[field_name]
return real_clean_function
class Form1(forms.Form):
clean_foo1 = clean_foo('foo1')
clean_foo2 = clean_foo('foo2')
class Form2(forms.Form):
clean_bar = clean_foo('bar')
-->8--
Additionally passing a parameter wouldn't break the current flexibility
present in clean_FIELD()-methods because other data in cleaned_data is
still accessible through the form-parameter. Anyway changing this would
simplify the most common use-case (and keeping half-cleaned data out of
cleaned_data will get easier).
And of course clean_FIELD() is not documented so far, which was the
reason for changing its name to do_clean_FIELD() in some commit lately.
So I don't see any reason why not to do this again to get things straight.
Greetings, David Danier
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en
-~--~~~~--~~--~--~---
Re: newforms issues
On 5/22/07, Gary Wilson <[EMAIL PROTECTED]> wrote: > > I would like to request comments/suggestions about a few patches I have > submitted recently for the newforms library that solve some issues that > I encountered during the development of some newforms. > > #3718 - newforms.Form.clean should have access to field errors [1] +1. form.errors isn't (necessarily) the exact complement of form.cleaned_data, so it makes sense to make it easy to get at. The only reason I can see for deferring availability of form.errors is the possibility of getting an incomplete error list in a clean_XXX function, but the same is true for cleaned_data - a note in the documentation should suffice for this problem. > #3896 - pass value to field specific clean function [2] Hrm. This is a tricky one. If this were a new API, clean_XXX(value) would make good sense. I'm also in favour of cleaned_data only having actual clean data in it. The backwards incompatibility is quite the sticking point, but I think the proposed API makes more sense, and if we're going to make the change, sooner would be better than later. Call me +0.5. I'd like to see this included, but I'll certainly defer to Adrian/Jacob on this one - after all, the status quo does work. +1 to Malcolm's suggestion on catching the ValidationError to clean up cleaned_data if the patch doesn't get blessed. > #4271 - Form should use a copy of data passed to it [3] -0. Its an expensive operation, and I'm not sure forms _should_ be messing with post data anyway. Besides, where are you going to put the code that removes the unwanted entries? The only place I can think of is in a custom __init__ method, in which case you're in your own code, so you can copy when required. If you really need this, you can do the copy manually, or write a filter that proxies the POST dictionary, removing 'bad' entries. > #4297 - make BaseForm's __errors attribute non-private [4] +1. IMHO, you need to have a good reason to make something private. I can't see any good reason why errors should be. Keep it _errors to discourage prying fingers, but don't prevent someone from doing something interesting that we haven't foreseen. Yours, Russ Magee %-) --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: newforms issues
On Mon, 2007-05-21 at 23:14 -0500, Gary Wilson wrote: > Malcolm Tredinnick wrote: > >> #3718 - newforms.Form.clean should have access to field errors [1] > > > > I'm not sure what this really gains. Fixing the second part of #3896 > > means that all the necessary information is in cleaned_data. I'm -0 on > > this, because I feel it's pretty pointless, merely providing more than > > one way to do something (not a good thing). > > > > Now convince me I'm wrong. What use-case have I overlooked? > > I would say that checking the errors property should be the one way to > see if there have been errors, not checking that every field in my model > has a key in cleaned_data. Without access to errors, you're going to > have one hell of an if statement with even a small form. Only if you code it badly. A for loop over self.cleaned_data works. > Add in > non-required fields and things get ugly even faster since it's not an > error if a non-required field has no key in cleaned_data. *shrug* In most cases you're going to have to access self.fields in any case. I'm still -0. But that really is -0 -- I have a slight preference, but I'll manage to keep on living whichever way it goes; quite happily, too. :-) > > Add in a subclass and now you've got the subclass having to know the > fields of the base class(es). Either that or a for loop looping through > self.base_fields to check that every field is in cleaned_data. Again, > getting even uglier with non-required fields. > > >> #3896 - pass value to field specific clean function [2] > > > > Grr...don't put two issues in one ticket! > > > > I'm -1 on the first part because it's an unnecessary backwards > > incompatibility change for the most part. It's not like it's a massive > > performance improvement in an area that is currently slow. > > > > Definitely +1 on the second part, though. Removing the first assignment > > to self.cleaned_data would make a lot of sense. > > I could have created a new ticket for the "self.cleaned_data retaining > the value even if clean_XXX() fails" problem, but by taking out the > first assignment to self.cleaned_data you *have to* pass the field's > value to clean_XXX() since the value would no longer be in > self.cleaned_data for clean_XXX() to access it. I figured opening a new > ticket would not be worthwhile, I could have been wrong. I'm sort of convinced. I'd forgotten that you would access the value through cleaned_data. We could still fix it in a backwards compatible fashion: if clean_ raises a ValidationError remove the value from cleaned_data, but maybe it's not worth it. I'm 0 on the first part and +1 on the second, then (since we can fix the second without the first). Malcolm --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: newforms issues
Malcolm Tredinnick wrote: >> #3718 - newforms.Form.clean should have access to field errors [1] > > I'm not sure what this really gains. Fixing the second part of #3896 > means that all the necessary information is in cleaned_data. I'm -0 on > this, because I feel it's pretty pointless, merely providing more than > one way to do something (not a good thing). > > Now convince me I'm wrong. What use-case have I overlooked? I would say that checking the errors property should be the one way to see if there have been errors, not checking that every field in my model has a key in cleaned_data. Without access to errors, you're going to have one hell of an if statement with even a small form. Add in non-required fields and things get ugly even faster since it's not an error if a non-required field has no key in cleaned_data. Add in a subclass and now you've got the subclass having to know the fields of the base class(es). Either that or a for loop looping through self.base_fields to check that every field is in cleaned_data. Again, getting even uglier with non-required fields. >> #3896 - pass value to field specific clean function [2] > > Grr...don't put two issues in one ticket! > > I'm -1 on the first part because it's an unnecessary backwards > incompatibility change for the most part. It's not like it's a massive > performance improvement in an area that is currently slow. > > Definitely +1 on the second part, though. Removing the first assignment > to self.cleaned_data would make a lot of sense. I could have created a new ticket for the "self.cleaned_data retaining the value even if clean_XXX() fails" problem, but by taking out the first assignment to self.cleaned_data you *have to* pass the field's value to clean_XXX() since the value would no longer be in self.cleaned_data for clean_XXX() to access it. I figured opening a new ticket would not be worthwhile, I could have been wrong. >> #4271 - Form should use a copy of data passed to it [3] > > Make up your mind! You want to save a single variable access in #3896, > but do a whole dictionary copy every time here??! :-) > > I'm a bit torn about this, because QueryDict.copy() isn't free and the > use-case you mention in the bug isn't particularly common and isn't > currently impossibly to do at the moment, by doing the copy() in the > Form constructor. > > I'm probably -0 for that reason. Not something I feel particularly > strongly about, though. > >> #4297 - make BaseForm's __errors attribute non-private [4] > > Whoops. I thought I'd already changed this one to Accepted. Apparently > I'd only got as far as making the decision in my head. I agree with you > on this. I couldn't think of a reason to hide that value (and making it > "_errors", rather than "errors" is the right change). > > I worry about people who want to hide away error messages after the > fact, rather than fixing the problems in the right place (whatever > validation generated the errors), but that's their problem. We'll > provided shotguns and bullets if it's not too hard, they can provide the > feet and entertain themselves at will. It's not all about hiding error messages though. If I had a Form subclass that had a full_clean() that called its parent and then did some other things and created errors, now the __errors dict would be split into two mangled instance attributes (one for each class). Not fun. I like my forms fully loaded, thank you. Gary --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: newforms issues
On 5/21/07, Malcolm Tredinnick <[EMAIL PROTECTED]> wrote: > > #3896 - pass value to field specific clean function [2] > > Grr...don't put two issues in one ticket! > > I'm -1 on the first part because it's an unnecessary backwards > incompatibility change for the most part. It's not like it's a massive > performance improvement in an area that is currently slow. > > Definitely +1 on the second part, though. Removing the first assignment > to self.cleaned_data would make a lot of sense. This change would make things cleaner and obvious (as well as being the best way to fix the bug of cleaned_data having uncleaned values). I personally don't mind changing code and retesting for that purpose, but since many others do I'm +0. But I wanted to point out that the two changes are closely related, and both are needed to fix the bug. Applying the second change without the first would break any code that uses a clean_* method right now, and applying the first without the second would also break code as well as not actually fixing the bug. I am not sure that I didn't miss something obvious, though. If I did, please correct me, I would like to properly understand the proposed change. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: newforms issues
On Tue, 2007-05-22 at 11:24 +1000, Malcolm Tredinnick wrote: > Hey Gary, > > On Mon, 2007-05-21 at 19:56 -0500, Gary Wilson wrote: [...] > > > #3896 - pass value to field specific clean function [2] > > Grr...don't put two issues in one ticket! > > I'm -1 on the first part because it's an unnecessary backwards > incompatibility change for the most part. It's not like it's a massive > performance improvement in an area that is currently slow. To be fair, I'll say I'm -0.5 on this change. Find somebody who's +1 and you're in the positive. It's not going to stop the world turning if we make the change and I'm possibly being overly-sensitive because I've been the recent target for people whining about backwards-incompat changes that were necessary. Regards, Malcolm --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: newforms issues
Hey Gary, On Mon, 2007-05-21 at 19:56 -0500, Gary Wilson wrote: > I would like to request comments/suggestions about a few patches I have > submitted recently for the newforms library that solve some issues that > I encountered during the development of some newforms. > > #3718 - newforms.Form.clean should have access to field errors [1] I'm not sure what this really gains. Fixing the second part of #3896 means that all the necessary information is in cleaned_data. I'm -0 on this, because I feel it's pretty pointless, merely providing more than one way to do something (not a good thing). Now convince me I'm wrong. What use-case have I overlooked? > #3896 - pass value to field specific clean function [2] Grr...don't put two issues in one ticket! I'm -1 on the first part because it's an unnecessary backwards incompatibility change for the most part. It's not like it's a massive performance improvement in an area that is currently slow. Definitely +1 on the second part, though. Removing the first assignment to self.cleaned_data would make a lot of sense. > #4271 - Form should use a copy of data passed to it [3] Make up your mind! You want to save a single variable access in #3896, but do a whole dictionary copy every time here??! :-) I'm a bit torn about this, because QueryDict.copy() isn't free and the use-case you mention in the bug isn't particularly common and isn't currently impossibly to do at the moment, by doing the copy() in the Form constructor. I'm probably -0 for that reason. Not something I feel particularly strongly about, though. > #4297 - make BaseForm's __errors attribute non-private [4] Whoops. I thought I'd already changed this one to Accepted. Apparently I'd only got as far as making the decision in my head. I agree with you on this. I couldn't think of a reason to hide that value (and making it "_errors", rather than "errors" is the right change). I worry about people who want to hide away error messages after the fact, rather than fixing the problems in the right place (whatever validation generated the errors), but that's their problem. We'll provided shotguns and bullets if it's not too hard, they can provide the feet and entertain themselves at will. Regards, Malcolm --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
newforms issues
I would like to request comments/suggestions about a few patches I have submitted recently for the newforms library that solve some issues that I encountered during the development of some newforms. #3718 - newforms.Form.clean should have access to field errors [1] #3896 - pass value to field specific clean function [2] #4271 - Form should use a copy of data passed to it [3] #4297 - make BaseForm's __errors attribute non-private [4] [1] http://code.djangoproject.com/ticket/3718 [2] http://code.djangoproject.com/ticket/3896 [3] http://code.djangoproject.com/ticket/4271 [4] http://code.djangoproject.com/ticket/4297 Thanks, Gary --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
