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.

Sign up for GitHub

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

Jump to bottom

Progressing #2704 #3076

Merged
merged 1 commit into from
Jun 25, 2015
Merged

Progressing #2704 #3076

merged 1 commit into from
Jun 25, 2015

Conversation

jpadilla
Copy link
Member

Don't merge this yet.

@jpadilla jpadilla mentioned this pull request Jun 25, 2015
Closed
@jpadilla jpadilla self-assigned this Jun 25, 2015
@tomchristie
Copy link
Member

Thanks for moving forward with this. Assume the upshot of this is to not call .all() on querysets in releationships but still try to ensure we don't revert #2602 in the process.

@jpadilla
Copy link
Member Author

@tomchristie just pushed a failing test to demo the revaluation issue from #2602. That obviously fails with the change to to_representation(), while our previous test passes. If we add revaluation of manager/queryset back, this last tests pases while our previous fails.

tomchristie
tomchristie reviewed Jun 25, 2015
View reviewed changes
tests/test_model_serializer.py

assert serializer.data == expected

def test_manager_queryset_revaluation(self):
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in an Issue2602TestCase class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, done.

@tomchristie
Copy link
Member

@jpadilla Okay, good progress, and basically resolved I think.

See here: #2602 (comment)

Essentially #2602 is invalid. We should remove the test for it, and merge this.

@tomchristie
Copy link
Member

We might want to think about if there's any ways we can guard against queryset caching between views being called, but don't know if it'll really be possible or not.

@tomchristie
Copy link
Member

Closes #2704

@tomchristie tomchristie added this to the 3.1.4 Release milestone Jun 25, 2015
@tomchristie
Copy link
Member

@jpadilla Think I'm happy with this now, ready to merge when you are.

@jpadilla
Copy link
Member Author

@tomchristie all yours. Thanks!

tomchristie added a commit that referenced this pull request Jun 25, 2015
@tomchristie
Merge pull request #3076 from jpadilla/issues/2704
df7c114 
Progressing #2704
@tomchristie tomchristie merged commit df7c114 into encode:master Jun 25, 2015
@tomchristie
Copy link
Member

Go team!

@tomchristie tomchristie modified the milestones: 3.1.4 Release, 3.2.0 Release Jul 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jpadilla jpadilla

Labels
Bug
Projects
None yet
Milestone
3.2.0 Release
Development

Successfully merging this pull request may close these issues.
2 participants
@jpadilla @tomchristie