Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow page splitting by parameter in pagination #166

Merged
merged 34 commits into from
Apr 3, 2024

Conversation

edelarua
Copy link
Contributor

@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2023

badge

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  ---------------------------------------------------------
R/paginate_listing.R        30       5  83.33%   81-86
R/rlistings_methods.R      101      16  84.16%   24-25, 39, 54, 58, 140-143, 146, 230-236
R/rlistings.R              178      26  85.39%   162-165, 168-171, 179, 212-216, 390-393, 397-400, 433-436
TOTAL                      309      47  84.79%

Diff against main

Filename                 Stmts    Miss  Cover
---------------------  -------  ------  -------
R/paginate_listing.R       -17      +4  -14.54%
R/rlistings_methods.R        0      +2  -1.98%
R/rlistings.R              +12      +1  +0.45%
TOTAL                       -5      +7  -2.47%

Results for commit: 2957e3b

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2023

Unit Tests Summary

  1 files   5 suites   8s ⏱️
 37 tests 29 ✅  8 💤 0 ❌
104 runs  91 ✅ 13 💤 0 ❌

Results for commit 2957e3b.

♻️ This comment has been updated with latest results.

@shajoezhu shajoezhu marked this pull request as draft October 14, 2023 06:21
@shajoezhu
Copy link
Collaborator

raised again by @BFalquet , @khatril perhaps, we can consider merge this in

@shajoezhu shajoezhu marked this pull request as ready for review March 1, 2024 02:12
@Melkiades Melkiades self-assigned this Mar 1, 2024
@edelarua edelarua marked this pull request as draft March 4, 2024 15:30
R/paginate_listing.R Outdated Show resolved Hide resolved
@edelarua edelarua marked this pull request as ready for review March 6, 2024 02:12
Copy link
Contributor

github-actions bot commented Mar 6, 2024

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
matrix_form 👶 $+0.20$ $+10$ $0$ $0$ $0$
paginate_listing 💔 $2.75$ $+2.05$ $+4$ $+1$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
export 👶 $+0.48$ export_as_txt_works_with_split_into_pages_by_var
listings 👶 $+0.03$ split_into_pages_by_var_works_as_expected
matrix_form 👶 $+0.21$ matrix_form_keeps_relevant_information_and_structure_about_the_listing
paginate_listing 💀 $0.02$ $-0.02$ defunct_is_defunct
paginate_listing 👶 $+0.18$ paginate_listing_works_with_split_into_pages_by_var

Results for commit 8248478

♻️ This comment has been updated with latest results.

R/rlistings.R Outdated Show resolved Hide resolved
R/paginate_listing.R Outdated Show resolved Hide resolved
R/paginate_listing.R Outdated Show resolved Hide resolved
edelarua and others added 5 commits March 28, 2024 10:29
Signed-off-by: Emily de la Rua <59304861+edelarua@users.noreply.github.com>
…ering/rlistings into 212_page_by_listings@main
NEWS.md Outdated Show resolved Hide resolved
@edelarua
Copy link
Contributor Author

@Melkiades it's looking good! I think it works well, thanks for all the work you've done so far!! :)

I'm also not sure what the best name would be - maybe ask around and see what people think?

@shajoezhu
Copy link
Collaborator

split_into_pages_by_var ssounds better, and people would understand

@Melkiades Melkiades merged commit 7ef01c2 into main Apr 3, 2024
23 checks passed
@Melkiades Melkiades deleted the 212_page_by_listings@main branch April 3, 2024 13:38
@Melkiades
Copy link
Contributor

@pawelru forgot to up the value of {formatters} sorry! I think we can have it in with next PR?

@pawelru
Copy link
Contributor

pawelru commented Apr 3, 2024

@pawelru forgot to up the value of {formatters} sorry! I think we can have it in with next PR?

yes please - the sooner the better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check and eventually add page_by functionality for pagination
4 participants