Submission: readODS #386

chainsawriot · 2020-06-29T17:53:38Z

Date accepted: 2022-06-24
Submitting Author Name: Chung-hong Chan
Submitting Author Github Handle: @chainsawriot
Repository: https://github.com/chainsawriot/readODS
Version submitted: 1.7.0
Editor: @noamross
Reviewers: @emmamendelsohn, @adamhsparks

Due date for @emmamendelsohn: 2020-08-11

Due date for @adamhsparks: 2020-08-11
Archive: TBD
Version accepted: TBD

Paste the full DESCRIPTION file inside a code block below:

Package: readODS
Type: Package
Title: Read and Write ODS Files
Version: 1.7.0
Date: 2020-06-22
Author: Gerrit-Jan Schutten, Chung-hong Chan, Thomas J. Leeper, John Foster,  and other contributors
Maintainer: Chung-hong Chan <chainsawtiney@gmail.com>
Contact: https://github.com/chainsawriot/readODS/
Description: Import ODS (OpenDocument Spreadsheet) into R as a data frame. Also support writing data frame into ODS file.
Imports:
    xml2 (>= 1.3.2),
    cellranger,
    readr (>= 1.2.1),
    stringi,
    utils
Suggests:
    dplyr,
    testthat,
    datasets,
    covr,
    knitr,
    rmarkdown
License: GPL-3
RoxygenNote: 7.1.0
Roxygen: list(markdown = TRUE)
VignetteBuilder: knitr

Scope

Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
- data retrieval
- data extraction
- data munging
- data deposition
- workflow automataion
- version control
- citation management and bibliometrics
- scientific software wrappers
- field and lab reproducibility tools
- database software bindings
- geospatial data
- text analysis
Explain how and why the package falls under these categories (briefly, 1-2 sentences):

This package reads and writes OpenDocument Spreadsheet (ods) files.

Who is the target audience and what are scientific applications of this package?

Researchers working with ods files

Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?

No

If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

Technical checks

Confirm each of the following by checking the box.

I have read the guide for authors and rOpenSci packaging guide.

This package:

does not violate the Terms of Service of any service it interacts with.
has a CRAN and OSI accepted license.
contains a README with instructions for installing the development version.
includes documentation with examples for all functions, created with roxygen2.
contains a vignette with examples of its essential functions and uses.
has a test suite.
has continuous integration, including reporting of test coverage using services such as Travis CI, Coveralls and/or CodeCov.

Publication options

Do you intend for this package to go on CRAN?
Do you intend for this package to go on Bioconductor?
Do you wish to automatically submit to the Journal of Open Source Software? If so:

JOSS Options

The package has an obvious research application according to JOSS's definition.
- The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
- The package is deposited in a long-term repository with the DOI:
- (Do not submit your package separately to JOSS)

Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:

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)
(Scope: Do consider MEE's Aims and Scope 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

I agree to abide by rOpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

The text was updated successfully, but these errors were encountered:

geanders · 2020-07-15T04:27:57Z

@chainsawriot : Thank you very much for your submission! @noamross will be your editor for the peer review process.

noamross · 2020-07-20T13:03:14Z

Hello @chainsawriot, thanks for your submission!

Editor checks:

Fit: The package meets criteria for fit and overlap
Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
License: The package has a CRAN or OSI accepted license
Repository: The repository link resolves correctly
Archive (JOSS only, may be post-review): The repository DOI resolves correctly
Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Here is the output of goodpractice::gp() with my comments. These are small but should be addressed in revision (they can wait until after reviews if you choose), and can be used by reviewers as entry points to the code. I am now seeking reviewers.

── GP readODS ────────────────────────────────────

It is good practice to

  ✖ write unit tests for all functions, and all package code  # Good coverage! 93% is fine.
    in general. 93% of code lines are covered by test cases.

    R/readODS.R:31:NA
    R/readODS.R:32:NA
    R/readODS.R:44:NA
    R/readODS.R:47:NA
    R/readODS.R:93:NA
    ... and 9 more lines

  ✖ omit "Date" in DESCRIPTION. It is not required and it
    gets invalid quite often. A build date will be added to the package
    when you perform `R CMD build` on it.
  ✖ add a "URL" field to DESCRIPTION. It helps users find
    information about your package online. If your package does not
    have a homepage, add an URL to GitHub, or the CRAN package package
    page.
  ✖ add a "BugReports" field to DESCRIPTION, and point it to
    a bug tracker. Many online code hosting services provide bug
    trackers for free, https://github.com, https://gitlab.com, etc.
  ✖ use '<-' for assignment instead of '='. '<-' is the
    standard, and R users and developers are used it and it is easier
    to read your code for them if you use '<-'.

    tests/testthat/test_legacy.R:16:9
    tests/testthat/test_legacy.R:21:9
    tests/testthat/test_legacy.R:24:9
    tests/testthat/test_legacy.R:26:9
    tests/testthat/test_legacy.R:28:9
    ... and 21 more lines

  ✖ avoid long code lines, it is bad for readability. Also,        # Some long lines are OK. In a few cases here they're indicative of a place where a utility functions might make nested code more readable .
    many people prefer editor windows that are about 80 characters
    wide. Try make your lines shorter than 80 characters

    R/readODS.R:28:1
    R/readODS.R:60:1
    R/readODS.R:66:1
    R/readODS.R:75:1
    R/readODS.R:76:1
    ... and 85 more lines

  ✖ avoid calling setwd(), it changes the global environment.
    If you need it, consider using on.exit() to restore the working   # You do! so this is fine.
    directory.

    R/writeODS.R:48:13
    R/writeODS.R:49:5
    R/writeODS.R:51:5

  ✖ avoid sapply(), it is not type safe. It might return a
    vector, or a list, depending on the input data. Consider using
    vapply() instead.

    R/readODS.R:153:26
    R/readODS.R:154:26
    R/readODS.R:184:24
    R/readODS.R:202:10
    R/readODS.R:310:12
    ... and 2 more lines

  ✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...),
    1:NROW(...) and 1:NCOL(...) expressions. They are error prone and
    result 1:0 if the expression on the right hand side is zero. Use
    seq_len() or seq_along() instead.

    R/readODS.R:20:12
    R/readODS.R:143:88
    tests/testthat/test_write_ods_append_update.R:33:43
    tests/testthat/test_write_ods_append_update.R:34:40
    tests/testthat/test_write_ods_append_update.R:68:49
    ... and 2 more lines

─────────────────────────────────────

Reviewer 1: @emmamendelsohn
Reviewer 2: @adamhsparks
Due date: 2020-08-11

noamross · 2020-07-21T12:23:11Z

Thanks @emmamendelsohn and @adamhsparks for agreeing to review! Due date: 2020-08-11.

adamhsparks · 2020-07-28T06:32:08Z

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

As the reviewer I confirm that there are no conflicts of interest for me to review this work

Documentation

The package includes all the following forms of documentation:

A statement of need clearly stating problems the software is designed to solve and its target audience in README
Installation instructions: for the development version of package and any non-standard dependencies in README
Vignette(s) demonstrating major functionality that runs successfully locally
Function Documentation: for all exported functions in R help
Examples for all exported functions in R Help that run successfully locally
Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

For packages co-submitting to JOSS

The package has an obvious research application according to JOSS's definition

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

Installation: Installation succeeds as documented.
Functionality: Any functional claims of the software been confirmed.
Performance: Any performance claims of the software been confirmed.
Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 5

Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

It's good to see the ODS format getting some attention like MS Excel's proprietary formats in R.
While ODS files may not be used as much, I still think it's important to support this file format.

I appreciate the simplicity of the package. It does two things and it does them well. A good UNIX philosophy. This will help keep the package maintainable into the future and help rio maintenance as well, I'm sure.

There are some areas that need to be addressed before it can be accepted into rOpenSci's software community though.

Following are my comments on what I'd like to see changed and what needs to be fixed for inclusion.

Documentation

Vignettes

I would state that PlantGrowth dataset comes from datasets before illustrating how to use it to help new users that aren't familiar with R and the available resources
For clarity, please specify that the command, write_ods(Plant_Growth, "plant.ods") will write the file in the current working directory of the user's session.

Function Documentation (and Other)

I would suggest to include a package level help file. usethis::use_package_doc() is a handy way to autogenerate the package-level documentation using details from the DESCRIPTION file.
Please include examples for use in the helpfile documentation for read_ods() and write_ods().
E-mail addresses in ROxygen should use a double @ sign, e.g. chainsawtiney@@gmail.com.
Please refer to http://r-pkgs.had.co.nz/man.html, "Roxygen comments".
Line 85 of writeODS.R, while technically correct, is unclear, especially for unfamiliar users. I'd suggest something like "An ODS file written to the file path location specified by the user."
Or something along those lines to be clear what the return value is in plain English.
Formatting the ROxygen comments properly will crosslink the HTML documentation, e.g. formatting "readr::type_convert" as readr::type_convert() on line 227 of readODS.R, will do this since you have set Roxygen: list(markdown = TRUE) in the DESCRIPTION file
write_ods() has a parameter, overwrite, I think you mean to use "deprecated" in place of "depreciated" in the documentation? This also appears on line 89 in readODS.R as "depreciated" instead of "deprecated".
For the parameters in write_ods() that take a TRUE/FALSE value, I'd like to see the default value to be explicitly stated here in the documentation.
Titles should be capitalised, "\title sections should be capitalized and not end in a period.", from https://developer.r-project.org/Rds.html.

Community Guidelines

The DESCRIPTION file lacks a 'BugReports' field. Most commonly this would point to https://github.com/chainsawriot/readODS/issues.
The contributing guidelines are missing either in CONTRIBUTING or in the README.

General Comments on Code

Check the output of goodpractice::gp() on the use of sapply() and 1:length() in functions to make the code more robust.
When checking the documentation, I relieved a warning message, Warning message: roxygen2 requires Encoding: UTF-8. Inserting Encoding: UTF-8 in DESCRIPTION fixes the issue.
The path parameter from read_ods() takes a default NULL value, but this is never checked. If the user does not supply a sheet to read the error message simply says, Error in .parse_ods_file(file) : object 'path' not found. This is honestly the most "base R" way of handling things I can think of. But I also think it's rather confusing to new users. This could be improved by a simple check to see if path is NULL before proceeding further when the function is executed. A more helpful message for the user would be desirable in this case as well, e.g. No file path was provided for the 'path' argument. Please provide a path to a file to import. or something of this nature to help guide the user.

I'm unclear on the functionality of the function on line 47 in writeODS.R.

.make_temp_dir <- function() {
    tmp <- tempfile()
    dir.create(tmp)
    return (tmp)
}

Would it not be more simple to just use the tempdir() command here and work in that temp directory for the zipping process? This is how I coded this functionality in the GSODR package to unzip files.

Consider using call. = FALSE in stop() functions throughout to provide a cleaner message to end-users.

Functionality

Installation

Installs as expected with no issues.

Functionality

Imports files and writes ODS files as advertised.
Compared with readxl::read_excel() it results in the exact same data.frame() in R.

Performance

Performance is notably slower for the same data than readxl::read_excel(), but considering that read_ods() is written in R while the other is C, it makes sense and this isn't designed for speed (nor advertised for it either).

Automated Tests

93% coverage is very good, most of the missing lines aren't easily covered and/or are related to stop() messages.

Packaging Guidelines

The assignment operators are not consistent between the functional code and the tests. The functions use <- while the tests use =. Per rOpenSci guidelines, either is acceptable, but you should be consistent in the use of them in the package.
Currently the package is only checked via CI on Travis for Linux. Using GitHub Actions and the examples from r-lib/actions, it's rather easy to set up a full check of all three major OSes and various R versions effectively. rOpenSci guidelines require that packages are tested against the latest, previous and development versions of R.

emmamendelsohn · 2020-08-11T00:23:35Z

Heads up I will have my review posted tomorrow!

emmamendelsohn · 2020-08-12T02:05:16Z

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

Briefly describe any working relationship you have (had) with the package authors.
As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

A statement of need clearly stating problems the software is designed to solve and its target audience in README
Installation instructions: for the development version of package and any non-standard dependencies in README
Vignette(s) demonstrating major functionality that runs successfully locally
Function Documentation: for all exported functions in R help
Examples for all exported functions in R Help that run successfully locally
Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

Installation: Installation succeeds as documented.
Functionality: Any functional claims of the software been confirmed.
Performance: Any performance claims of the software been confirmed.
Automated tests: Unit tests cover essential functions of the package
and a reasonable range of inputs and conditions. All tests pass on the local machine.
Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 5

Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

Well done--nice and simple! The code was easy to follow and review. I only have a few comments to add to what has already been mentioned.

Documentation

Consider mentioning the list_ods_sheets() function in the vignette/README. I sometimes use readxl::excel_sheets(), so I imagine the equivalent would be useful for .ods users.
Perhaps not worth mentioning in the vignettes, but the formula_as_formula argument in read_ods() would have been useful when I had to QA/QC spreadsheets in my previous job :)
In documentation for read_ods(), I suggest clarifying that the na argument can be set to NULL, resulting in empty cells being treated as NA. And perhaps rephrase "Character vector of strings to use for missing values" to "Character vector of strings to be replaced as NA".

Functionality

Check lines 92-98 in .parse_rows(). read_ods("starwars.ods", skip = 11) returns an empty data frame, when it should return the full data frame and a warning that the skip value is greater than the number of rows. The same happens when skip = 12. This is because in starwars.ods, the last two rows from parsed_sheets do not contain any values but are still counted toward length(rows), resulting in FALSE for the condition on line 92.
Not required, but you may consider giving warnings for identical and missing column names. If the user were to convert the data.frame to a tibble, these naming issues will result in errors.

noamross · 2020-10-05T13:03:39Z

@chainsawriot Just following up here, please let us know when you've been able to address the reviews from @adamhsparks and @emmamendelsohn. (Thanks to both of you!)

chainsawriot · 2020-10-05T13:11:34Z

@noamross

First of all, I would like to thank both @adamhsparks and @emmamendelsohn for their insightful comments.

I am really sorry that I didn't aware of the 2 weeks responding window as specified in 7.3. Although it is now overdue, may I still ask for an extension? I will submit a revised version of the package by the end of this month.

Thank you very much.

noamross · 2020-10-05T13:27:58Z

That's fine, @chainsawriot. We're all doing our best and are trying to be accommodating with schedules this challenging year.

noamross · 2020-11-23T13:56:56Z

Hi @chainsawriot, I just wanted to follow up to see when you expect to be able to move forward.

noamross · 2021-02-22T20:29:07Z

Hello, @chainsawriot. I am putting a "holding" label on this. Do let us know if and when you intend to return to it.

chainsawriot · 2021-11-30T11:09:26Z

Hello, I've managed to update readODS according to the reviewers' comments. Sorry for the super long delay.

May I first address @emmamendelsohn comments?

Documentation

Consider mentioning the list_ods_sheets() function in the vignette/README. I sometimes use readxl::excel_sheets(), so I imagine the equivalent would be useful for .ods users.

The usage of list_ods_sheets() is added.

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/vignettes/overview.Rmd#L89-L94

Perhaps not worth mentioning in the vignettes, but the formula_as_formula argument in read_ods() would have been useful when I had to QA/QC spreadsheets in my previous job :)
In documentation for read_ods(), I suggest clarifying that the na argument can be set to NULL, resulting in empty cells being treated as NA. And perhaps rephrase "Character vector of strings to use for missing values" to "Character vector of strings to be replaced as NA".

The documentation is revised as suggested.

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L255-L256

Functionality

Check lines 92-98 in .parse_rows(). read_ods("starwars.ods", skip = 11) returns an empty data frame, when it should return the full data frame and a warning that the skip value is greater than the number of rows. The same happens when skip = 12. This is because in starwars.ods, the last two rows from parsed_sheets do not contain any values but are still counted toward length(rows), resulting in FALSE for the condition on line 92.

This one is tricky because the XML content in an ODS file can also record the empty lines after the "actual content". I've tried many ways to detect those empty lines but the detection is not straightforward. I took a simple solution: don't test whether skip is further than the "actual content". Instead, return an empty data frame when skip goes into any empty area (instead of ignoring skip and returning the entire data frame; which is not quite natural anyway). This new behavior is made explicit in the documentation. It could create breakage, but I think most users would not use skip for skipping lines at the very end but at the very beginning.

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L257

Not required, but you may consider giving warnings for identical and missing column names. If the user were to convert the data.frame to a tibble, these naming issues will result in errors.

Exporting tibble is a planned feature of the v2.0 of readODS. In that release, it is planned to use tibble's .name_repair option. I took the liberty of your "Not required" and defer this to the planned v2.0.

chainsawriot · 2021-11-30T12:32:41Z

@adamhsparks 's comments

Vignettes

I would state that PlantGrowth dataset comes from datasets before illustrating how to use it to help new users that aren't familiar with R and the available resources

I've made it explicit that PlantGrowth is from datasets

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/vignettes/overview.Rmd#L25

For clarity, please specify that the command, write_ods(Plant_Growth, "plant.ods") will write the file in the current working directory of the user's session.

As suggested.

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/vignettes/overview.Rmd#L25

Function Documentation (and Other)

I would suggest to include a package level help file. usethis::use_package_doc() is a handy way to autogenerate the package-level documentation using details from the DESCRIPTION file.

A package-level help file is added

https://github.com/chainsawriot/readODS/blob/master/man/readODS-package.Rd

Please include examples for use in the helpfile documentation for read_ods() and write_ods().

Some examples are now included in the helpfile documentation for read_ods() and write_ods()

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L268-L276

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/writeODS.R#L82-L88

E-mail addresses in ROxygen should use a double @ sign, e.g. chainsawtiney@@gmail.com.

All e-mail addresses are fixed.

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/writeODS.R#L81

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L324

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L267

Line 85 of writeODS.R, while technically correct, is unclear, especially for unfamiliar users. I'd suggest something like "An ODS file written to the file path location specified by the user." Or something along those lines to be clear what the return value is in plain English.

Revised as suggested.

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/writeODS.R#L80

Formatting the ROxygen comments properly will crosslink the HTML documentation, e.g. formatting "readr::type_convert" as readr::type_convert() on line 227 of readODS.R, will do this since you have set Roxygen: list(markdown = TRUE) in the DESCRIPTION file

Revised as suggested

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L254

write_ods() has a parameter, overwrite, I think you mean to use "deprecated" in place of "depreciated" in the documentation? This also appears on line 89 in readODS.R as "depreciated" instead of "deprecated".

(Sorry for the embarrassing typo) It has been corrected throughout the documentation.

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L309

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L336

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L355

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/writeODS.R#L79

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/writeODS.R#L92

For the parameters in write_ods() that take a TRUE/FALSE value, I'd like to see the default value to be explicitly stated here in the documentation.

Thanks, @mmahmoudian for the PR
8dff1603c9154187562ae09402363307c363d97b

Titles should be capitalised, "\title sections should be capitalized and not end in a period.", from https://developer.r-project.org/Rds.html.

Revised as suggested

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L245

Community Guidelines

The DESCRIPTION file lacks a 'BugReports' field. Most commonly this would point to https://github.com/chainsawriot/readODS/issues.

BugsReports field is added to DESCRIPTION

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/DESCRIPTION#L9

The contributing guidelines are missing either in CONTRIBUTING or in the README.

Contribution guidelines are added to the README.

https://github.com/chainsawriot/readODS/blame/master/README.md#L211

General Comments on Code

Check the output of goodpractice::gp() on the use of sapply() and 1:length() in functions to make the code more robust.

All instances of sapply() are replaced by their brothers and sisters from purrr. All 1:length() instances are replaced by seq or seq_len accordingly.

When checking the documentation, I relieved a warning message, Warning message: roxygen2 requires Encoding: UTF-8. Inserting Encoding: UTF-8 in DESCRIPTION fixes the issue.

The encoding for roxy2 is made explicit.

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/DESCRIPTION#L27

The path parameter from read_ods() takes a default NULL value, but this is never checked. If the user does not supply a sheet to read the error message simply says, Error in .parse_ods_file(file) : object 'path' not found. This is honestly the most "base R" way of handling things I can think of. But I also think it's rather confusing to new users. This could be improved by a simple check to see if path is NULL before proceeding further when the function is executed. A more helpful message for the user would be desirable in this case as well, e.g. No file path was provided for the 'path' argument. Please provide a path to a file to import. or something of this nature to help guide the user.

The missing path is now checked.

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/readODS.R#L280-L282

I'm unclear on the functionality of the function on line 47 in writeODS.R. Would it not be more simple to just use the tempdir() command here and work in that temp directory for the zipping process? This is how I coded this functionality in the GSODR package to unzip files.

Yes, it should be replaced by a simple tempdir().

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/R/writeODS.R#L101

Consider using call. = FALSE in stop() functions throughout to provide a cleaner message to end-users.

All stop() instances are modified with call. = FALSE

Installation

Installs as expected with no issues.

Functionality

Imports files and writes ODS files as advertised.
Compared with readxl::read_excel() it results in the exact same data.frame() in R.

Performance

Performance is notably slower for the same data than readxl::read_excel(), but considering that read_ods() is written in R while the other is C, it makes sense and this isn't designed for speed (nor advertised for it either).

I address the performance issue by stating in the README about the non-optimized performance. Also, alternatives (such as using headless Libreoffice) are suggested.

https://github.com/chainsawriot/readODS/blame/master/README.md#L168

Automated Tests

93% coverage is very good, most of the missing lines aren't easily covered and/or are related to stop() messages.

Packaging Guidelines

The assignment operators are not consistent between the functional code and the tests. The functions use <- while the tests use =. Per rOpenSci guidelines, either is acceptable, but you should be consistent in the use of them in the package.

Due to contributions from different contributors who use different styles, the programming style in the codebase was mixed. The whole package is now using the consistent style of "<-".

Currently the package is only checked via CI on Travis for Linux. Using GitHub Actions and the examples from r-lib/actions, it's rather easy to set up a full check of all three major OSes and various R versions effectively. rOpenSci guidelines require that packages are tested against the latest, previous and development versions of R.

The package is now checked with GHA for release, oldrel and devel.

https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/.github/workflows/R-CMD-check.yaml#L21-L25

chainsawriot · 2021-11-30T12:33:28Z

@noamross Once again, I am really sorry for the great delay. Please let me know what I should do next. Thank you very much!

chainsawriot · 2022-06-16T22:05:49Z

@noamross I was wondering will there be any update on this?

noamross · 2022-06-17T18:23:27Z

Hi @chainsawriot, sorry that this slipped through the cracks. @adamhsparks and @emmamendelsohn, please respond to let us know whether the changes address all your concerns.

adamhsparks · 2022-06-18T02:10:25Z

@noamross, @chainsawriot, I'm happy with the changes

emmamendelsohn · 2022-06-24T17:31:55Z

Everything looks good from my end as well. @chainsawriot @noamross

noamross · 2022-06-24T17:48:58Z

@ropensci-review-bot approve readODS

ropensci-review-bot · 2022-06-24T17:49:00Z

Approved! Thanks @chainsawriot for submitting and @emmamendelsohn, @adamhsparks for your reviews! 😁

To-dos:

Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You will need to enable two-factor authentication for your GitHub account.
This invitation will expire after one week. If it happens write a comment @ropensci-review-bot invite me to ropensci/<package-name> which will re-send an invitation.
After transfer write a comment @ropensci-review-bot finalize transfer of <package-name> where <package-name> is the repo/package name. This will give you admin access back.
Fix all links to the GitHub repo to point to the repo under the ropensci organization.
Delete your current code of conduct file if you had one since rOpenSci's default one will apply, see https://devguide.ropensci.org/collaboration.html#coc-file
If you already had a pkgdown website and are ok relying only on rOpenSci central docs building and branding,
- deactivate the automatic deployment you might have set up
- remove styling tweaks from your pkgdown config but keep that config file
- replace the whole current pkgdown website with a redirecting page
- replace your package docs URL with https://docs.ropensci.org/package_name
- In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar, https://github.com/ropensci/foobar
Fix any links in badges for CI and coverage to point to the new repository URL.
Increment the package version to reflect the changes you made during review. In NEWS.md, add a heading for the new version and one bullet for each user-facing change, and each developer-facing change that you think is relevant.
We're starting to roll out software metadata files to all rOpenSci packages via the Codemeta initiative, see https://docs.ropensci.org/codemetar/ for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.
You can add this installation method to your package README install.packages("<package-name>", repos = "https://ropensci.r-universe.dev") thanks to R-universe.

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent).

Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @ropensci/blog-editors in your reply. She will get in touch about timing and can answer any questions.

We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding (with advice on releases, package marketing, GitHub grooming); the guide also feature CRAN gotchas. Please tell us what could be improved.

Last but not least, you can volunteer as a reviewer via filling a short form.

chainsawriot · 2022-06-24T20:08:11Z

@ropensci-review-bot finalize transfer of readODS

ropensci-review-bot · 2022-06-24T20:08:14Z

Transfer completed.
The readODS team is now owner of the repository and the author has been invited to the team

- Post acceptance changes, Change README, DESCRIPTION with ropensci Github links - Remove COC - Bump version

geanders added the 1/editor-checks label Jul 1, 2020

geanders assigned noamross Jul 15, 2020

noamross added 2/seeking-reviewer(s) 3/reviewer(s)-assigned and removed 1/editor-checks 2/seeking-reviewer(s) labels Jul 20, 2020

chainsawriot mentioned this issue Jul 29, 2020

Issues from ropensci/software-review#386 ropensci/readODS#70

Closed

noamross added 4/review(s)-in-awaiting-changes and removed 3/reviewer(s)-assigned labels Oct 5, 2020

noamross added the holding label Feb 22, 2021

mmahmoudian mentioned this issue May 5, 2021

Fix for TRUE / FALSE in #70 ropensci/readODS#73

Merged

noamross added 5/awaiting-reviewer(s)-response and removed holding 4/review(s)-in-awaiting-changes labels Jun 17, 2022

ropensci-review-bot added 6/approved and removed 5/awaiting-reviewer(s)-response labels Jun 24, 2022

ropensci-review-bot closed this as completed Jun 24, 2022

chainsawriot added a commit to ropensci/readODS that referenced this issue Jun 24, 2022

Add ropensci related chages, ref ropensci/software-review#386 (#86)

7dfde6e

- Post acceptance changes, Change README, DESCRIPTION with ropensci Github links - Remove COC - Bump version

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Submission: readODS #386

Submission: readODS #386

chainsawriot commented Jun 29, 2020 •

edited by ropensci-review-bot

Loading

geanders commented Jul 15, 2020

noamross commented Jul 20, 2020 •

edited

Loading

noamross commented Jul 21, 2020

adamhsparks commented Jul 28, 2020 •

edited

Loading

For packages co-submitting to JOSS

emmamendelsohn commented Aug 11, 2020

emmamendelsohn commented Aug 12, 2020

noamross commented Oct 5, 2020

chainsawriot commented Oct 5, 2020

noamross commented Oct 5, 2020

noamross commented Nov 23, 2020

noamross commented Feb 22, 2021

chainsawriot commented Nov 30, 2021

chainsawriot commented Nov 30, 2021

chainsawriot commented Nov 30, 2021

chainsawriot commented Jun 16, 2022

noamross commented Jun 17, 2022

adamhsparks commented Jun 18, 2022

emmamendelsohn commented Jun 24, 2022

noamross commented Jun 24, 2022

ropensci-review-bot commented Jun 24, 2022

chainsawriot commented Jun 24, 2022

ropensci-review-bot commented Jun 24, 2022

Submission: readODS #386

Submission: readODS #386

Comments

chainsawriot commented Jun 29, 2020 • edited by ropensci-review-bot Loading

Scope

Technical checks

Publication options

Code of conduct

geanders commented Jul 15, 2020

noamross commented Jul 20, 2020 • edited Loading

Editor checks:

Editor comments

noamross commented Jul 21, 2020

adamhsparks commented Jul 28, 2020 • edited Loading

Package Review

Documentation

For packages co-submitting to JOSS

Functionality

Final approval (post-review)

Review Comments

Documentation

Vignettes

Function Documentation (and Other)

Community Guidelines

General Comments on Code

Functionality

Installation

Functionality

Performance

Automated Tests

Packaging Guidelines

emmamendelsohn commented Aug 11, 2020

emmamendelsohn commented Aug 12, 2020

Package Review

Documentation

Functionality

Final approval (post-review)

Review Comments

Documentation

Functionality

noamross commented Oct 5, 2020

chainsawriot commented Oct 5, 2020

noamross commented Oct 5, 2020

noamross commented Nov 23, 2020

noamross commented Feb 22, 2021

chainsawriot commented Nov 30, 2021

Documentation

Functionality

chainsawriot commented Nov 30, 2021

Vignettes

Function Documentation (and Other)

Community Guidelines

General Comments on Code

Installation

Functionality

Performance

Automated Tests

Packaging Guidelines

chainsawriot commented Nov 30, 2021

chainsawriot commented Jun 16, 2022

noamross commented Jun 17, 2022

adamhsparks commented Jun 18, 2022

emmamendelsohn commented Jun 24, 2022

noamross commented Jun 24, 2022

ropensci-review-bot commented Jun 24, 2022

chainsawriot commented Jun 24, 2022

ropensci-review-bot commented Jun 24, 2022

chainsawriot commented Jun 29, 2020 •

edited by ropensci-review-bot

Loading

noamross commented Jul 20, 2020 •

edited

Loading

adamhsparks commented Jul 28, 2020 •

edited

Loading