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

TA comments #1

Open
ChadFibke opened this issue Nov 18, 2018 · 0 comments
Open

TA comments #1

ChadFibke opened this issue Nov 18, 2018 · 0 comments

Comments

@ChadFibke
Copy link

Coding Style

The good 👍

  • Functions are nicely annotated!

  • Good is readable.

Needs work 👎

  • NA

Coding Strategy

The good 👍

  • Good use of @examplesto provide documentation and examples in one place for some of your new functions!

  • Good use of in function checks. Using stop() provides users with informative error messages! However I don't see that this is used in all of your functions.

  • Good use of Test_that to test your new functions! In addition to validity, testthat also allows us to make the test parameters once, and easily store, and reuse them when our function/package is updated.

Needs work 👎

  • Please use more informative commit messages. Having "a" will not help any collaborators understand any changes you have made.

  • Your Vignettes for the foofactors package are not showing up on as a link in your package index. (Your package index is the overarching description file when you search for your package in the package tab and then click on your package name). I think you may have forgot to run the following code because I don’t see a inst/doc directory in your repo (if this subdirectory is present roxygen will link the vignettes to your index file):

use_vignette("vignette_name") 
## knit the vignette_name.Rmd
build_vignettes()  
  • Your README contains the examples for your package, but most R users do not look at the github. They usually download the package from CRAN and play with the data in R. For example did you look at ggplot2 or dplyr's github accounts before downloading? It is always good to have everything bundled so that people who work in R can have all the information they need by navigating the package description.

  • It would be a good idea to use @importFrom readr write_csv to allow your package to use the write_csv function from the readr package. That way the @ will communicate this to the documentation and namespace file! Same with stats::reorder and dplyr::desc.

  • Some of your documentation does not match the use of multiple functions (newfactor,dfread). Your newfactor() says the argument f will accept the factor, but your function actually uses x. Also your dfread() documentation takes a dataframe, but your actual function only takes a file and a level. If you had larger packages this would become a huge issue!

  • You did use testthat, but there are a diverse set of expect_* functions that could more holistically test your code instead of testing one expect_equal.

Graphs & Tables

The good 👍

  • NA

Needs work 👎

  • NA

Creativity & Ease of Access

The good 👍

  • Easy navigation from canvas to your repo.

  • The README is populated with informative details about installation and function use. Also shows the new functions in action!

Needs work 👎

  • The README for a package should explain the contents of the package, and how to install, and also include a selling point! Why is your package better than using base r? You do explain some of the contents, but fail to explain what the foofactors.Rcheck directory is, and you do not have a selling point that builds off of Jenny's point! The additional selling point should reflect what your additional functions do.

  • I wad unable to instal your project with your directions and and got the following error:

> devtools::install_github("STAT545-UBC-students/hw07-RyanGao67/foofactor")
#Downloading GitHub repo STAT545-UBC-students/hw07-RyanGao67@master
#from URL https://api.github.com/repos/STAT545-UBC-students/hw07-RyanGao67/zipball/master
#Installation failed: Does not appear to be an R package (no DESCRIPTION)

So I looked into your DESCRIPTION file (which You have). I looked into the problem and the DESCRIPTION file crashed either because it did not have Type: Package, or because you added the additional line to the Author@R. I was able to install the package by cloning it onto my laptop > install your packaged locally using devtools::install("../students/hw07-RyanGao67/foofactors"). Then it started to work!

  • Your levels_reorder.txt, and test_reorder.csv should not be in the testtht folder. They should be kept in a data folder.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant