Skip to content

Conversation

@iorlas
Copy link
Contributor

@iorlas iorlas commented Apr 8, 2015

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.

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
Copy link
Contributor

xordoquy commented Apr 8, 2015

@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 xordoquy closed this Apr 8, 2015
@iorlas
Copy link
Contributor Author

iorlas commented Apr 8, 2015

@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
Copy link
Contributor

xordoquy commented Apr 8, 2015

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

@xordoquy xordoquy reopened this Apr 8, 2015
@iorlas
Copy link
Contributor Author

iorlas commented Apr 8, 2015

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
Copy link
Contributor Author

iorlas commented Apr 9, 2015

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

@iorlas
Copy link
Contributor Author

iorlas commented Apr 9, 2015

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
Copy link
Contributor Author

iorlas commented Apr 10, 2015

😢

@xordoquy
Copy link
Contributor

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

@lovelydinosaur
Copy link
Contributor

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.

@lovelydinosaur
Copy link
Contributor

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
Copy link
Contributor Author

iorlas commented Apr 20, 2015

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
Copy link
Contributor

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

@iorlas
Copy link
Contributor Author

iorlas commented Apr 21, 2015

@jpadilla done :)

lovelydinosaur added a commit that referenced this pull request Apr 21, 2015
Now it is possible to display viewset w/o paginator
@lovelydinosaur lovelydinosaur merged commit 605369e into encode:master Apr 21, 2015
@lovelydinosaur
Copy link
Contributor

Ta.

@iorlas
Copy link
Contributor Author

iorlas commented Apr 21, 2015

Hooray! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants