Navigation Menu

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 #2

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

TA comments #2

ChadFibke opened this issue Nov 18, 2018 · 0 comments

Comments

@ChadFibke
Copy link

Coding Style

The good 👍

  • New functions are nicely annotated!

  • Code is readable!

Needs work 👎

  • NA

Coding Strategy

The good 👍

  • Good use of in function checks. Using stop() in a conditional statements provides users with informative error messages, and terminates the code!

  • Some of the documentation is clear and helpful! Documentation parameters line up with the arguments used in the function!

  • Good use of @examples to provide documentation and examples in one place!

  • 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. Furthermore, you used a more diverse set of expect_* functions that holistically test your code instead of testing expect_identicle multiple times. 🔥

  • Yay no fatal errors when I run check()

Needs work 👎

  • The Read_textFile() is not adding anything to read.delim()... Considering beefing it up by incorporate useful concepts like package dependencies and more robust/dynamic/flexible coding practice. What I mean by flexible is writing functions that give the user more control like in your Write_textFile() function that allows the user to choose what they can change instead of hard coding in values. Also consider looking into using an "..." once there are to many function arguments.

  • 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, which holds all your vignettes md/Rmd/html that are linked to your package documentation :

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.

  • Some of your documentation does not match the usage of the function. For example in Read_textFile() the usage is just Read_textFile(), then you also provide additional arguments below. There is also a similar problem with Write_textFile(). of df_read(). If you had larger packages this would become a huge issue because some of the documentation says one thing, and the other part of the documentation say another on how to use your function.

  • Be more consistent with your documentation. Some of your documents have the proper parameters, usage, and examples, while others do not!

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, AND thank you for adding some additional sales pitch as to why your package is better than base R! ⭐

Needs work 👎

  • When I go to your actual git hub the package folder is inactive. Did you drag and drop this file directly onto github? A couple of students and I had the same issue, but if you would have made a hw07 like normal and cloned it to your laptop > went to Jenny's repo and clicked on clone/download > download zip > unzip the folder > move that unzipped folder to your cloned repo the foofactors package would have been active.

  • The testthat file should not have any datasets in that directory. You should have a directory called data where your datasets are stored. once stored in the data directory you could get the test that function to navigate to the data folder to find the data set of interest!

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