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

Cleanup '.model' shortcut from code and docs #2486

Merged
merged 2 commits into from Jan 29, 2015

Conversation

maryokhin
Copy link
Contributor

@maryokhin maryokhin commented Jan 29, 2015

As far as I understand, if it was removed, then mentioning it in the docs will just confuse people.

@tomchristie
Copy link
Member

tomchristie commented Jan 29, 2015

Looks good, yup! Tho presumably some digging needed to figure out why the tests are failing.

@maryokhin
Copy link
Contributor Author

maryokhin commented Jan 29, 2015

Yes, I'm surprised I managed to fail the whole build with a 2 line change o_O

@maryokhin
Copy link
Contributor Author

maryokhin commented Jan 29, 2015

Ok, I really don't get what's up. Anyone has any suggestions?

@tomchristie
Copy link
Member

tomchristie commented Jan 29, 2015

@maryokhin Do the tests run okay for you locally?

@maryokhin
Copy link
Contributor Author

maryokhin commented Jan 29, 2015

@tomchristie no, have the same issue locally, so it's probably my miss somewhere, just don't get how that's possible in 2 lines of code >_<


assert model_cls, '`base_name` argument not specified, and could ' \
assert queryset, '`base_name` argument not specified, and could ' \
Copy link
Member

@tomchristie tomchristie Jan 29, 2015

Choose a reason for hiding this comment

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

Should we instead be using assert queryset is not None here? Are we accidentally evaluating the queryset?

Copy link
Contributor Author

@maryokhin maryokhin Jan 29, 2015

Choose a reason for hiding this comment

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

Yes, that was the issue, assert was evaluating the queryset, whose implementation fetches from the DB if it's not in the cache. Correct implementation exposed some old tests using .model, so I'll rebase and fix those along the way too.

Copy link
Member

@tomchristie tomchristie Jan 29, 2015

Choose a reason for hiding this comment

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

Fab.

@tomchristie
Copy link
Member

tomchristie commented Jan 29, 2015

One inline comment with a possibility. Failing that I guess narrow this down to single step changes. (Roll back to the original, run the tests for sanity, then just change one line of a code at a time, running the tests after each change)

@tomchristie tomchristie added this to the 3.0.5 Release milestone Jan 29, 2015
@jpadilla
Copy link
Member

jpadilla commented Jan 29, 2015

This looks good to me now.

tomchristie added a commit that referenced this pull request Jan 29, 2015
Cleanup '.model' shortcut from code and docs
@tomchristie tomchristie merged commit 2a43d9d into encode:master Jan 29, 2015
maryokhin added a commit to maryokhin/django-rest-framework that referenced this pull request Feb 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants