Closed olivroy closed 1 year ago
Merging #554 (100c832) into main (b2700cf) will not change coverage. Report is 1 commits behind head on main. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## main #554 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 1183 1184 +1
=========================================
+ Hits 1183 1184 +1
Files Changed | Coverage Ξ | |
---|---|---|
R/tabyl.R | 100.00% <100.00%> (ΓΈ) |
Fixing the bug and the package restructuring seem like unrelated things to me. I think we can do both in one PR though since that's how it is. But I'm gonna think about them one at a time π
Nice catch on the bug! If I follow correctly it's only when show_missing_levels = FALSE
, because when that's TRUE then tidyr::complete
fills in the zeroes?
I'm kind of aghast not to have had a test in place for that already. I think your test is correct but to keep it leaner I think we can reuse this object from line 251 of test-tabyl.R
:
x <- data.frame(
a = c(1, 2, 2, 2, 1, 1, 1, NA, NA, 1),
b = c(rep("up", 4), rep("down", 4), NA, NA),
c = 10,
d = c(NA, 10:2),
stringsAsFactors = FALSE
)
And then the new test could be
x %>% tabyl(a, b, show_missing_levels = FALSE)
Can you check my thinking on this @olivroy ?
Also would you like to add yourself as a contributor in the package DESCRIPTION? You have been making big contributions!
Hi @sfirke , thanks for the review!
Fixing the bug and the package restructuring seem like unrelated things to me. I think we can do both in one PR though since that's how it is. But I'm gonna think about them one at a time π
Sorry about that. It's just that when I used devtools::build_readme()
it made an unexpected diff change (currently on main). Since it was a quick fix, I thought I'd squeeze it in. but I don't mind separating if you'd like.
I'm kind of aghast not to have had a test in place for that already. I think your test is correct but to keep it leaner I think we can reuse this object from line 251 of test-tabyl.R:
I agree! I wanted to have a smaller test, so that's a good one (I verified that it indeed fails on main) Deleted my long test, and put that one instead!
Also would you like to add yourself as a contributor in the package DESCRIPTION? You have been making big contributions!
Sure! thank you very much. It's an honor. I love the pkg!
Looks great! Sorry to go quiet on this, I sent you my feedback right as I went on vacation and then lost it in the shuffle. Thank you for contributing this!
Hi, yet another cleanup PR. Addresses #549
It helped unveil a bug that I introduced in #552 (sorry), so I made these corrections, along with a new test in 9e70421aa53648bf9b8c3f51a14da4320063b8aa (if there is a shorter way to make this test, I'd be happy to improve)
Currently, if you run
devtools::build_readme()
in the main branch, you see an unexpected diff.But the main point of this PR is to remove duplication by making README.md the home page of the pkgdown website.
I checked the difference between
index.md
andREADME.md
withHere is a list of the differences I found:
Already there (seems to have been updated in README, but not in index.md)
index.md: Advanced R users can already do everything covered here README.md: Advanced R users can perform many of these tasks already **
index.md: Getting Started README.md: Installation
index.md: Contact Me README.md: Contact me
index.md: Adorning tabyls README.md #### Adorning tabyls
Better code indentation in README already
Rationale
Most of the info is either duplicated or outdated in index.md
Remove old Travis badges Remove badges from the pkgdown site (only for GitHub home)
The font awesome icons are ignored in the GitHub version, so they can be safely added to
Last cosmetic changes I made to README
Code indentation Changed devtools for remotes since devtools is quite heavy, just for installing a dev version. Suggest r-universe install since it's precompiled binaries and easier than github for installing dev version.
Finally, I did a visual inspection with
Before and after seems fine.