UBC-MDS / software-review-2022

0 stars 0 forks source link

Submission Group 02: rlyrics (R) #21

Open artanzand opened 2 years ago

artanzand commented 2 years ago

Submitting Author Name: Artan Zandian Submitting Author Github Handle: @artanzand Other Package Authors Github handles: @MacyChan, @manju-abhinandana, @abhiket Repository: https://github.com/UBC-MDS/rlyrics Version submitted: 1.0.0 Submission type: Standard Editor: @artanzand Language: en Reviewer 1: Cici Du @ciciecho-ds Reviewer 2: Sanchit Singh @ssingh90 Reviewer 3: Shi Yan Wang @sy25wang


Package: rlyrics
Title: Downloads, cleans and creates wordcloud of lyrics
Version: 0.0.0.9000
Authors@R: c(
    person("Abhiket","Gaurav", , "abhiketg@student.ubc.ca", role = c("aut")),
    person("Artan","Zandian", , "aazandian@student.ubc.ca", role = c("aut")),
    person("Macy","Chan", , "macychan@student.ubc.ca", role = c("aut", "cre")),
    person("Manju","Abhinandana Kumar", , "manju2na@student.ubc.ca", role = c("aut")))
Description: Downloads data from Kaggle, finds the lyrics of a certain artist and song, cleans the lyrics and creates wordcloud of lyrics.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.2.9000
Suggests: 
    knitr,
    rmarkdown,
    covr,
    testthat (>= 3.0.0)
Config/testthat/edition: 3
Imports: 
    tm,
    textclean,
    readr,
    here,
    rvest,
    stringr,
    wordcloud2,
    kaggler (>= 0.0.0.9000),
    dplyr,
    tidyselect
Remotes: 
    ldurazo/kaggler
VignetteBuilder: knitr

Scope

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

ciciecho-ds commented 2 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:

Functionality

Estimated hours spent reviewing: 1


Review Comments

Great idea and I loved your presentation in 542!

  1. test() runs fine but check() returns 1 error that says test failures: Error: '......tests/testthat/data/lyrics_22.txt' does not exist. You might want to address this issue.
  2. Since to use this package, you have to use Kaggle API, I suggests adding more documentation and link to the Kaggle relevant documentation for users. https://www.kaggle.com/docs/api
  3. I think it would make more logical sense if in your examples, you continue to use one song as your data source as it is a sequential process instead of extracting the lyrics from one song and cleaning another text.
  4. The returned word cloud image is in portrait. I wonder if it's possible to generate it in landscape so it may look better.
  5. Your README doesn't show the word cloud image. I think it would be more appealing if you included it in your README.
MacyChan commented 2 years ago

Cici, Thank you for the valuable review. We have addressed serval comments and the changes will be included in the upcoming release.

  1. Fixed check() error
  2. Added extra Kaggle API information on ReadMe and Vignette
  3. Added cloud image on ReadMe
  4. We will discuss the extra feature of word cloud image about cloud image size options.

Understood that it might be more helpful with lyrics example under clean_text(), however, we have tested that out and the result looks messy. Therefore, we decided to keep it as is. For a better explanation, feel free to visit this article

sy25wang commented 2 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:

Functionality

Estimated hours spent reviewing: 1 hour


Review Comments

Overall I think this is a well structured package serving its purpose. Some of my suggestions and recommendations are:

  1. Please review docstrings for your functions. Some function examples are in the following format that I am not sure if this is on purpose? `#' @examples

    ' \dontrun{`

  2. Under the Functions section of your README, the input column and the description together can lead to some confusion. For example, for clean_text function, the input mentions text, but the description refers to the lyrics.
  3. I am curious how some incorrect inputs are handled by your functions. For example, how the clean_text function handles the input that is a combination of words and digits (e.g., Se7en)
  4. For your testing, would you consider adding more edge case testing? For example, what if the background_color input is of type character but not a real color word? I would recommend considering and adding more if cases to your functions and test cases.
  5. The requirement of using Kaggle API may restrict the uses of this package. I believe the dataset is not limited to Kaggle but can be from other sources? You may want to add to your example paragraph that Kaggle here is only for example illustration.
AndyYang80 commented 2 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:

Functionality

Estimated hours spent reviewing: 1


Review Comments

  1. It may be a good idea to include additional edge test cases in the clean_text function. Including tests for more foreign characters can improve the reliability of the function
  2. Including functionality to check for songs with multiple artists and multiple versions of artists names (including capital letters) can improve the scope of the package to include a wider variety of songs
  3. Adding a specific error output telling the user to fix their kaggle credentials in cases where there is difficulty authenticating kaggle credentials or finding the json file might be a useful addition to the download data function
  4. I believe dding "\dontrun" is to avoid check() issues withing the docstrings, but it may be a good idea to enable these functions in the future to run with check() without issues since this will make the package more robust
Sanchit120496 commented 2 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:

Functionality

Estimated hours spent reviewing: 1 hr


Review Comments

Great work. Love the concept that you combine functionalities of many packages into one.

  1. Docstring of extract_lyrics has "\dontrun{" as a typo
  2. The r code styling can be kept constant such as using "<-" instead of "="
  3. Similar to python package, keeping most of the info of the functions in the features section rather than example section makes more sense
  4. These days, we have a number of songs with the same name. The package can include those kinds of cases in their package too, to differentiate between them
  5. The package might not take numbers as ong names which can be a problem
  6. The test cases are built very nicely, covering all possible scenarios