search

UBC-MDS / software-review-2021

1 stars 1 forks source link

Submitted : labzenr (R) #11

Open sukh2929 opened 3 years ago

kmoravej commented 3 years ago

Submitting Author:

Repository: https://github.com/UBC-MDS/labzenr Version submitted: 1.0.0 Editor: TBD Reviewers: TBD

Archive: TBD Version accepted: TBD

Package: labzenr
Title: A package to help UBC MDS students manage their labs
Version: 1.0.0
Authors@R: 
    c(person(given = "Kamal",
           family = "Moravej",
           role = c("aut", "cre"),
           email = "sk.moravej@gmail.com"),
      person(given = "Rafael",
           family = "Pilliard-Hellwig",
           email = "rtaph@student.ubc.ca",
           role = c("aut")),
      person(given = "Sukhdeep",
             family = "Kaur",
             email = "Sukhdeep_2929@yahoo.com",
             role = c("aut")))
Description: labzenr is a R package that adds 
    more zen to your student experience of working on MDS labs. 
    It lets you manage common tasks such as counting total marks 
    in an assigment, and performs common checks for mechanics in 
    your iPython notebooks and R markdown assignments.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.1
URL: https://ubc-mds.github.io/labzenr, https://github.com/UBC-MDS/labzenr
BugReports: https://github.com/UBC-MDS/labzenr/issues
Suggests: 
    covr,
    checkmate,
    credentials,
    knitr,
    mockery,
    rmarkdown,
    testthat (>= 3.0.0)
Config/testthat/edition: 3
Imports: 
    fs,
    stringi,
    usethis,
    gert,
    magrittr,
    janitor,
    dplyr,
    tibble,
    tidyr,
    jsonlite,
    stringr,
    rlang,
    purrr,
    glue,
    withr,
    readr,
    rappdirs
VignetteBuilder: knitr

Scope

  • To the authors' knowledge, no package yet exists in the R ecosystem that serves this specific purpose. However, several existing packages will be used to power the functionality of labzenr, including gert and usethis.

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

MEE Options - [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)

Code of conduct

chuangw6 commented 3 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [ ] A short summary describing the high-level functionality of the software
  • [ ] Authors: A list of authors with their affiliations
  • [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Estimated hours spent reviewing:

Review Comments

Hi Team, Congratulations on building and releasing your first R package! First of all, I want to say this is such a great initiative and I really like the functionalities your package include, which makes really easier for MDS students to check their lab performance and reflect on themselves. Please see the detailed comments below.

  1. Thank you for carefully designing the tests. I've run all the tests and checked the package and confirm that all tests have passed and the check for the R package is passed. However, the current code coverage of your tests is 95.98%. Given this is a relatively small package, I think more tests can be added to cover all lines of your functions. For example, the if statement on line 38 in the check_commits.R file has been hit 7 times, but the if body is never tested. Also, both if body and else body are not tested between lines 54-59 in the check_commits.R file.
  2. Your code format is so great and the coding style is consistent throughout the whole package. All the functions are well-documented. The possible improvement is that it would be great if you use bullet points in some long descriptions for a return/parameter type i.e. the description for the return part of signature_ls function. Also, it's better to include the parameter type in the function specification i.e. parameter of repo in signature_ls function and other functions.
  3. In terms of the documentation inside the implementation of a function, all functions did a great job by containing some brief in-line comments, which can really help reviewers and the future version of yourself to understand what a particular line or a block of code is doing. I really like this point.
  4. Just a minor point that there some typos on your package vignette website i.e. creditials, assigment. But as the documentation website can be an important media via which users can get a better understanding of your package and decide whether or not to use the package, it would be great if these typos can be avoided.
  5. It's great to see you use some helper functions in your package to make your code neat and clean. But I think the documentation for helper functions can be hidden from your package vignette website as users might not have a chance to use them or try to understand them. I just found a possible way to do this. See here for more details.
  6. As check_mechanics is the key final function which calls all other major functions in the package, I think it might be good to put this function in a separate script.
  7. It's great to see you specified the package you want to use for a function by using :: syntax. This is a great practice for developing a package. Great work.

Overall, the functions in this package work properly and I really like the purpose of the function. I will definitely use the package for my MDS courses. Thanks!

Best regards

Reviewer: Chuang Wang

JuntingHe commented 3 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [ ] A short summary describing the high-level functionality of the software
  • [ ] Authors: A list of authors with their affiliations
  • [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Estimated hours spent reviewing: 2

Review Comments

General Comments

Hi labzenr team,

Congratulation on completing such an amazing package in a short time! This package is really creative and useful and I would like to use it for monitoring my labs in the next block. I really appreciate the concise description for the usages of this package and the clear documentation of all of the functions. Additionally, the automatic test passed, and the functions are well implemented.

Specific Comments

There are a few errors I encountered when I reviewed the package in the following:

Functionality:

Vignettes:

There are a few comments which may be useful for improving the package in the following:

User interface:

Vignettes:

Function documentation:

In conclusion, the package is well-documented and well-implemented. I really think it would be useful for the student experience of working on UBC Master of Data Science labs.

Best, Junting

sukh2929 commented 3 years ago

Hi @JuntingHe,

Thanks for your valuable feedback! We are glad that you liked our idea of the package.

Our team will connect and discuss the errors and further improvement based on your suggestions.

sukh2929 commented 3 years ago

Hi @chuangw6 ,

Thanks for your valuable feedback! We are glad to hear that you liked it and you would also use this!

Our team will connect and discuss further improvement based on your suggestions.

rtaph commented 3 years ago

Hi @JuntingHe,

Thanks again for your review!

We'll respond thoroughly to all your points later, but for now, could you try to upgrade readr to the latest version (1.4.0 or later)? I suspect the error you encountered has to do with a breaking change when readr switched to version 1.4.0.

If upgrading fixes it for you, we can add the readr version dependency in our DESCRIPTION file.

rtaph commented 3 years ago

Dear @chuangw6 ,

Thank you for having taken the time to thoroughly review our package!

Here are some inline replies:

  1. Thank you for carefully designing the tests. I've run all the tests and checked the package and confirm that all tests have passed and the check for the R package is passed. However, the current code coverage of your tests is 95.98%. Given this is a relatively small package, I think more tests can be added to cover all lines of your functions. For example, the if statement on line 38 in the check_commits.R file has been hit 7 times, but the if body is never tested. Also, both if body and else body are not tested between lines 54-59 in the check_commits.R file.

💯 % coverage is the goal! We were pretty exhausted after writing a very complex git coverage suite so by the deadline, we settled for the high 90s 😅 But this is certainly something to come back to!

  1. Your code format is so great and the coding style is consistent throughout the whole package. All the functions are well-documented. The possible improvement is that it would be great if you use bullet points in some long descriptions for a return/parameter type i.e. the description for the return part of signature_ls function. Also, it's better to include the parameter type in the function specification i.e. parameter of repo in signature_ls function and other functions.

Great suggestion! Fixed here.

  1. In terms of the documentation inside the implementation of a function, all functions did a great job by containing some brief in-line comments, which can really help reviewers and the future version of yourself to understand what a particular line or a block of code is doing. I really like this point.

Thanks! Glad that our code was clear 😊

  1. Just a minor point that there some typos on your package vignette website i.e. creditials, assigment. But as the documentation website can be an important media via which users can get a better understanding of your package and decide whether or not to use the package, it would be great if these typos can be avoided.

Great point! We've reviewed the docs and run spelling::spell_check_package() on our package. See PR #53

  1. It's great to see you use some helper functions in your package to make your code neat and clean. But I think the documentation for helper functions can be hidden from your package vignette website as users might not have a chance to use them or try to understand them. I just found a possible way to do this. See here for more details.

Agreed. We've scaffolded check_commits(), check_lat_version(), check_repo_link(), parse_lab(), and find_assignment() so that they are no longer exported functions. Some of the other functions, like signature_*(), should still be exposed to the users.

See PR #54.

  1. As check_mechanics is the key final function which calls all other major functions in the package, I think it might be good to put this function in a separate script.

Done! See PR #55.

Overall, the functions in this package work properly and I really like the purpose of the function. I will definitely use the package for my MDS courses. Thanks!

Thanks for the valuable input! With your permission, we would love to add you to our contributors.md file as a reviewer. Would you like us to add you? If so, how should we credit you and what would be the email address listed?

Best,

Raf, Kamal, and Sukhdeep.

sukh2929 commented 3 years ago

Dear @JuntingHe,

Thank you so much for your valuable insights!

Here are some point-by-point replies:

Specific Comments

There are a few errors I encountered when I reviewed the package in the following:

Functionality:

  • I tried to run labzenr::check_mechanics() in my DSCI_554_lab4 repo and I got this error: Error in readr::write_csv(sigs, file = sigfile) : unused argument (file = sigfile)

Sorry about that!

We suspect that this may be because you might not be using readr version 1.4.0 or higher. Could you please check and let us know?

In the mean time, we have made the dependency requirement stricter.

  • I tried to test the function find_sigfile manually and I got this error again: `Error in readr::write_csv(sigs, file = sigfile) : unused argument (file = sigfile)

Probably the same issue as above with readr. They changed their parameter in read_csv() from path = to file =.

Vignettes:

  • I tried to follow the steps and run labzenr::signatures_ls(), and I got this error: Error: 'signatures_ls' is not an exported object from 'namespace:labzenr'

Good catch!

That is a typo. It should have said labzenr::signature_ls() (no 's'). We've fixed that here in the vignette.

  • I tried to run signature_add("John S(\\.|mith)", is_user = TRUE), and I got this error: Error in readr::write_csv(sigs, file = sigfile) : unused argument (file = sigfile)

This also looks like the issue with the readr version

  • The output for check_mechanics() function took me some time to understand. It would be nice to have output in a cleaned table, which is similar to that of the count_points() function.

We discussed it as a team, and we would prefer to keep the messages rather than a table. The main reason for is that we would like the function to work well at the command line.

However, we hear you about it being confusing! We'll revisit the documentation on this (see issue #51)

Vignettes:

  • Some typos were noticed on the vignette website such as creditials (credentials).

Good catch! Fixed in PR #53.

Function documentation:

  • I was a little confused about the margin input of the extract_points() function. It might be helpful if its description is more detailed.

We'll make this clearer in the coming days (issue #52).

In conclusion, the package is well-documented and well-implemented. I really think it would be useful for the student experience of working on UBC Master of Data Science labs.

Thanks so much for taking 2 hours to review our work 🎉. Our 📦 is better as a result of your inputs.

With your permission, we would love to add you to our DESCRIPTION as a reviewer, as well as in our contributors.md file. How would you like us to write your name and email? Do you have an ORCID?

Best,

Sukhdeep, Kamal, and Raf