Cleanup '.model' shortcut from code and docs by maryokhin · Pull Request #2486 · encode/django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation10 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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

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

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

@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 >_<

tomchristie


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

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?

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.

Choose a reason for hiding this comment

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

Fab.

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)