-
Notifications
You must be signed in to change notification settings - Fork 132
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
Make README.md the home index of the pkgdown site + fix unintended change in tabyl()
#554
Conversation
Add a test for a case in README that was broken in a previous PR.
Codecov Report
@@ Coverage Diff @@
## main #554 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 1183 1184 +1
=========================================
+ Hits 1183 1184 +1
|
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 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
And then the new test could be
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!
Sorry about that. It's just that when I used
I agree! I wanted to have a smaller test, so that's a good one (I verified that it indeed fails on main)
Sure! thank you very much. It's an honor. I love the pkg! |
tabyl()
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 9e70421 (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.