geanders
commented
4 years ago @chainsawriot : Thank you very much for your submission! @noamross will be your editor for the peer review process.
Closed chainsawriot closed 2 years ago
geanders
commented
4 years ago @chainsawriot : Thank you very much for your submission! @noamross will be your editor for the peer review process.
noamross
commented
4 years ago Hello @chainsawriot, thanks for your submission!
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
commented
4 years ago Thanks @emmamendelsohn and @adamhsparks for agreeing to review! Due date: 2020-08-11.
adamhsparks
commented
4 years ago 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
The package includes all the following forms of documentation:
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.mdmatching 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).
Estimated hours spent reviewing: 5
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.
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.
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.
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.
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.
Installs as expected with no issues.
Imports files and writes ODS files as advertised.
Compared with readxl::read_excel() it results in the exact same data.frame() in R.
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).stop() messages.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
commented
4 years ago Heads up I will have my review posted tomorrow!
emmamendelsohn
commented
4 years ago 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
The package includes all the following forms of documentation:
URL, BugReports and Maintainer (which may be autogenerated via Authors@R).Estimated hours spent reviewing: 5
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.
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".
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
commented
3 years ago @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
commented
3 years ago @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
commented
3 years ago That's fine, @chainsawriot. We're all doing our best and are trying to be accommodating with schedules this challenging year.
noamross
commented
3 years ago Hi @chainsawriot, I just wanted to follow up to see when you expect to be able to move forward.
noamross
commented
3 years ago Hello, @chainsawriot. I am putting a "holding" label on this. Do let us know if and when you intend to return to it.
chainsawriot
commented
2 years ago Hello, I've managed to update readODS according to the reviewers' comments. Sorry for the super long delay.
May I first address @emmamendelsohn comments?
The usage of list_ods_sheets() is added.
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.
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.
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
commented
2 years ago @adamhsparks 's comments
I've made it explicit that PlantGrowth is from datasets
As suggested.
A package-level help file is added
https://github.com/chainsawriot/readODS/blob/master/man/readODS-package.Rd
Some examples are now included in the helpfile documentation for read_ods() and write_ods()
All e-mail addresses are fixed.
Revised as suggested.
Revised as suggested
(Sorry for the embarrassing typo) It has been corrected throughout the documentation.
Thanks, @mmahmoudian for the PR 8dff1603c9154187562ae09402363307c363d97b
Revised as suggested
BugsReports field is added to DESCRIPTION
https://github.com/chainsawriot/readODS/blob/cfd484e1baf1c8da9013d48fa9fb7f150adb3bc9/DESCRIPTION#L9
Contribution guidelines are added to the README.
https://github.com/chainsawriot/readODS/blame/master/README.md#L211
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.
The encoding for roxy2 is made explicit.
The missing path is now checked.
Yes, it should be replaced by a simple tempdir().
All stop() instances are modified with call. = FALSE
Imports files and writes ODS files as advertised.
Compared with readxl::read_excel() it results in the exact same data.frame() in R.
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
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 "<-".
The package is now checked with GHA for release, oldrel and devel.
chainsawriot
commented
2 years ago @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
commented
2 years ago @noamross I was wondering will there be any update on this?
noamross
commented
2 years ago 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
commented
2 years ago @noamross, @chainsawriot, I'm happy with the changes
emmamendelsohn
commented
2 years ago Everything looks good from my end as well. @chainsawriot @noamross
noamross
commented
2 years ago @ropensci-review-bot approve readODS
ropensci-review-bot
commented
2 years ago Approved! Thanks @chainsawriot for submitting and @emmamendelsohn, @adamhsparks for your reviews! :grin:
To-dos:
@ropensci-review-bot invite me to ropensci/<package-name> which will re-send an invitation.@ropensci-review-bot finalize transfer of <package-name> where <package-name> is the repo/package name. This will give you admin access back.pkgdown website and are ok relying only on rOpenSci central docs building and branding,
pkgdown website with a redirecting pagehttps://docs.ropensci.org/package_nameURL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar, https://github.com/ropensci/foobarcodemetar::write_codemeta() in the root of your package.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
commented
2 years ago @ropensci-review-bot finalize transfer of readODS
ropensci-review-bot
commented
2 years ago Transfer completed.
The readODS team is now owner of the repository and the author has been invited to the team
Date accepted: 2022-06-24 Submitting Author Name: Chung-hong Chan Submitting Author Github Handle: !--author1-->@chainsawriot<!--end-author1-- Repository: https://github.com/chainsawriot/readODS Version submitted: 1.7.0 Editor: !--editor-->@noamross<!--end-editor-- Reviewers: @emmamendelsohn, @adamhsparks
Due date for @emmamendelsohn: 2020-08-11 Due date for @adamhsparks: 2020-08-11Archive: TBD
Version accepted: TBD
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.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
This package reads and writes OpenDocument Spreadsheet (ods) files.
Researchers working with ods files
No
Technical checks
Confirm each of the following by checking the box.
This package:
Publication options
JOSS Options
- [ ] The package has an **obvious research application** according to [JOSS's definition](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [ ] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) 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*)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