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.

Sign up for GitHub

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

Jump to bottom

Update paginate_listing function to handle lpp=NULL. Adding unit test… #60

Merged
merged 14 commits into from
Jan 30, 2023
Merged

Update paginate_listing function to handle lpp=NULL. Adding unit test… #60

merged 14 commits into from
Jan 30, 2023

Conversation

ayogasekaram
Copy link
Contributor

… for lpp=NULL.

closes #53

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022 •
edited
Loading

badge

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  ---------------------------------
R/paginate_listing.R        49       1  97.96%   54
R/rlistings_methods.R       97      13  86.60%   23-24, 32, 43, 92, 97-98, 184-190
R/rlistings.R              128      11  91.41%   265-268, 272-275, 297, 301-302
TOTAL                      274      25  90.88%

Diff against main

Filename                Stmts    Miss  Cover
--------------------  -------  ------  -------
R/paginate_listing.R       +2      -1  +2.21%
TOTAL                      +2      -1  +0.43%

Results for commit: 13c6f25

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@ayogasekaram ayogasekaram removed the request for review from edelarua December 6, 2022 16:31
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022 •
edited
Loading

Unit Tests Summary

  1 files    3 suites   0s ⏱️
10 tests 10 ✔️ 0 💤 0 ❌
54 runs  54 ✔️ 0 💤 0 ❌

Results for commit 73dc547.

♻️ This comment has been updated with latest results.

Melkiades
Melkiades approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@Melkiades Melkiades left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! :)

@Melkiades Melkiades removed the request for review from insights-engineering-bot December 6, 2022 17:25
@Melkiades Melkiades added the bug Something isn't working label Dec 6, 2022
gmbecker
gmbecker requested changes Dec 6, 2022
View reviewed changes
R/paginate_listing.R Outdated Show resolved Hide resolved
tests/testthat/test-paginate_listing.R Outdated Show resolved Hide resolved
tests/testthat/test-paginate_listing.R Outdated Show resolved Hide resolved
R/paginate_listing.R Show resolved Hide resolved
@gmbecker gmbecker mentioned this pull request Dec 6, 2022
Closed
@shajoezhu shajoezhu added the sme label Dec 7, 2022
@Melkiades
Copy link
Contributor

@ayogasekaram can you update this PR with the latest requested changes? Thanks! :)

gmbecker
gmbecker requested changes Dec 20, 2022
View reviewed changes
Copy link
Collaborator

@gmbecker gmbecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change (don't use attr directly). If ncol doesn't do what you need, you can do length(listing_dispcols(obj)) to see how many listing columns (including key columns) there are.

Other than that, looks great.

tests/testthat/test-paginate_listing.R Outdated Show resolved Hide resolved
@gmbecker
Copy link
Collaborator

@ayogasekaram also please be sure to merge in main as I just merged a small pr fixing a different bug

@gmbecker
Copy link
Collaborator

@cicdguy it did it again here. Leaving it so you can see it but @ayogasekaram you should consider this PR merged and the task completed

@gmbecker
Copy link
Collaborator

@cicdguy it did it again here. Leaving it so you can see it but @ayogasekaram you should consider this PR merged and the task completed
Screen Shot 2022-12-20 at 4 05 37 PM

@ayogasekaram ayogasekaram mentioned this pull request Dec 21, 2022
Closed
@Melkiades
Copy link
Contributor

@ayogasekaram, we can merge this now imo

@Melkiades Melkiades merged commit 822b298 into main Jan 30, 2023
@Melkiades Melkiades deleted the 53_lpp_is_null@main branch January 30, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmbecker gmbecker gmbecker approved these changes

@Melkiades Melkiades Melkiades approved these changes

Assignees
No one assigned
Labels
bug Something isn't working sme
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error when lpp = NULL
5 participants
@ayogasekaram @Melkiades @gmbecker @shajoezhu @edelarua