Now it is possible to display viewset w/o paginator

[iorlas]

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.

[xordoquy]

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

[iorlas]

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

[xordoquy]

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

[iorlas]

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...

[iorlas]

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

[iorlas]

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]

😢

[xordoquy]

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

[tomchristie]

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.

[tomchristie]

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.

[iorlas]

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?

[jpadilla]

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

[iorlas]

@jpadilla done :)

[tomchristie]

Ta.

[iorlas]

Hooray! 😄