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

Conversation15 Commits1 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 }})

Since pagination is now included in every generic viewset, we should have ability to disable it and we have it: paginator=None or pagination_class=None. But this piece of code relies on existence of property instead of its value.

@iorlas if view.paginator exists and its value to None, the paginator will be set to None.
Without a failing test case I don't get what this change brings in.

@xordoquy with paginator=None DRF API interface gives exception:

File "...rest_framework/renderers.py", line 614, in get_context
    if hasattr(view, 'paginator') and view.paginator.display_page_controls:
AttributeError: 'NoneType' object has no attribute 'display_page_controls'

Is it a good practice to close issue just because you don't believe me and you want me to write some tests? That's rude.

@iorlas I'm sorry I was rude.
It is usually a good practice to provide a failing test case to help maintainers to understand the issue and ensure it won't regress. Trust is subjective while test cases are not.
We try to keep the opened issues count as low as possible therefore we sometime close issues fast.
Given the snippet you provided, I don't see a reason this shouldn't be reopened.

Yeah, I understand it. I'm trying right now to write a test for this issue. It isn't easy for me since I'm not really familiar with tests in python world <- shame on me -_-

Anyway, browsable API has really low amount of tests, so it is trickier than should be...

Yea, I got it, wait few minutes before I'll push it

So here we go. I know I should commit test before fix so you can see that test is failing, should I create new branch to make it possible?

@iorlas sorry, you could have more help on irc.

I'm happier to see this change in without tests.
Sometimes a code change is just correct.
Your change is correct - let's remove the tests and accept it.

Also hope no offense is taken @iorlas - we deal with a huge number of tickets - we need to prefer proactively closing them and then later reopening if we're wrong.

If we don't do that we end up with a huge backlog of open issues that may not truly reflect the things that we think it's important for us to be working on.

Nah, it's fine, I know what it feels like :) I was just afraid that this will be forgotten :( So, should I remove tests from branch?

@iorlas yeah, just remove the tests from your branch and this pull request will be updated.

tomchristie added a commit that referenced this pull request

Apr 21, 2015

Now it is possible to display viewset w/o paginator

This was referenced

Mar 9, 2017

This was referenced

Oct 6, 2017

This was referenced

Oct 16, 2017

This was referenced

Nov 6, 2017

This was referenced

Nov 14, 2017

This was referenced

Dec 10, 2017

This was referenced

Dec 20, 2017