#1810 Fix odd pagination error
Merged by pingou. Opened by vibhcool.
vibhcool/pagure fix_count  into  master

Download 1810.patch

Fixes: #1790 Odd pagination on issue search

Tested and open for review :)

1 new commit added

  • fix pep8 error

pagure.lib.search_issue() can return either a issue object, list of issues or number of issues. It can also be modified to return only list of issues whether there is 1 issue or more than 1 . Number of issues can be returned using len() method. This way shall be faster and simpler, isn't it?

How would, retrieving potentially a large list of items from the database and then going over it to know its size be faster than retrieving a single integer from the database? :)

That indentation shouldn't be changed actually, pep8 won't like it :)

I'd like to test this, but from a quick look it looks good :)

IMHO it might, in fact, be easier to do issues_cnt = len(issues) here since we have already performed the DB query for getting the issues.

The same holds true for similar instances of issues_cnt above.

Is that correct?

Excpet the one comment, rest looks good to me. Well done! :thumbsup:

1 new commit added

  • adjust indentation

made the changes, up for review :)

@pingou retrieving single integer from database shall be faster but in the function, pagure.ui.issues.view_issue() method, to get issues and to get no. of issues for issues and issues_cnt, oth_issues and oth_issues_cnt , total_issues and total_issues_cnt , pagure.lib.search_issue() is used for them all. Instead issues_cnt and oth_issues_cnt can be found using len(issues) and len(oth_issues) respectivly.

Excpet the one comment, rest looks good to me. Well done! 👍

@cep
thats what i am asking if I shall use len().

Thanks :)

Looks good, let's rebase and merge :)

Would you like to work on unit-tests for this?

rebased

@pingou Thank you , rebased yes , i want to work on unit-tests for this :D ,

open for merging :)

If you want to work on the unit-tests, then let's keep this open and do a review when the tests are there and passing and merge it all at once :)

If you want to work on the unit-tests, then let's keep this open and do a review when the tests are there and passing and merge it all at once :)

yes , sure, on it :)

Here are two patches that I'm proposing to fix this PR:

0001-Take-into-account-the-number-of-items-per-page-from-.patch

0002-Fix-the-pagination-when-showing-all-the-issues.patch

https://paste.fedoraproject.org/551843/57210148

Made the proposedchanges, tested and open for review :)

rebased

These three changes are wrong, there should be two blank lines and no spaces :)

Oops , I made some editing errors :P

1 new commit added

  • fix spaces error

Thanks :)

Pull-Request has been merged by pingou

Metadata