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 capability to camelize QuerySets #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add capability to camelize QuerySets #15

wants to merge 1 commit into from

Conversation

MattBlack85
Copy link

This PR is intended to add support when one is using .values() method on querysets. Actual behaviour is returning the QuerySet since it's not a dict, a list neither a tuple. Now a ValuesQuerySet (deprecated from 1.9, for this reason I check straight for QuerySet instance) can be camelized correctly.

Since there is no django setup while running tests I'm not sure how to test it and add this behaviour, but if you have any suggestion @vbabiy I'd be happy to do it

new_list = []
for i in range(len(data)):
new_list.append(camelize(data[i]))
return new_list
Copy link

Choose a reason for hiding this comment

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

Isn’t this the same as return [camelize(item) for item in queryset]?

Copy link
Author

Choose a reason for hiding this comment

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

@merwok yes, of course

Copy link

Choose a reason for hiding this comment

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

I see now that this line was inspired by line 21/23 above, where the existing list is modified in place (which is broken for tuples, debatable for lists and already discussed in other tickets).

@vbabiy
Copy link
Owner

vbabiy commented Jul 7, 2017

@MattBlack85 this is great idea, we need to set up an integration suite to test this. Also allowing us to test settings.

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