Reworked ErrorDict.as_json() to prevent unnecessary serialization/deseri... #2379

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Member

loic commented Feb 28, 2014

...alization step.

Thanks @apollo13 for the suggestion. Refs #17413.

django/forms/utils.py
@@ -86,7 +86,7 @@ class ErrorList(UserList, list):
def as_data(self):
return self.data
- def as_json(self, escape_html=False):
+ def as_json(self, escape_html=False, prepare_only=False):
@alex

alex Feb 28, 2014

Member

It seems much simpler to split as_json into two functions, one which returns a string with JSON, and another which returns a json-serializable structure; rather than putting a new flag on this.

@loic

loic Feb 28, 2014

Member

We discussed this moments ago with @apollo13 on IRC, I'm a bit concerned by the number of dict like objects floating around.

There are Form.errors (ErrorDict which maps fields to rendered strings), as_data() (maps fields to ValidationError), .data which is a private API, .as_json() (serialized as_data() minus the ValidationError's params), and we'd be adding one more for json-serializable formatting. Folks were already fairly confused by the situation which lead to the clarification in 2e4200b.

That said, I don't mind implementing it if provided with a good name for the method.

Edit: Sorry, linked the wrong commit.

Owner

apollo13 commented Mar 1, 2014

@loic Since @alex also think it's better; I think we should rework it. Would it be possible for as_json on ErrorDict to just reuse the results from as_data?

EDIT:// Sry for non-inline answering, hit the wrong button :þ

Member

loic commented Mar 1, 2014

Quick response from mobile. Either ways as_data() is where the information is at. We serialize a subset of it (we leave out the params). We do need to normalize the output of as_data() though because a user fiddling with the content of ErrorList may store simple strings. ErrorList.as_json() did such normalization for us.

I wonder if as_data() should always return instances of ValidationError.

Member

loic commented Mar 1, 2014

Another two tentative patches:

https://github.com/loic/django/compare/pr2379_take2 which duplicates the preparation of the data in ErrorDict, this is akin to the other as_methods() which don't call their ErrorList counterpart.

https://github.com/loic/django/compare/pr2379_take3 which introduces an ErrorList.get_json_data() method.

Both patches move the normalization of strings / ValidationError to the ErrorList.as_data() method.

Personally I prefer the original approach as it's a bit less invasive, but it's a close call.

Reworked ErrorDict.as_json() to prevent unnecessary serialization/des…
…erialization step.

Thanks @apollo13 for the suggestion. Refs #17413.
Owner

apollo13 commented Mar 6, 2014

After talking to Alex I've commited take3 as 34236ef -- Thank you!

@apollo13 apollo13 closed this Mar 6, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment