Submission: exoplanets - Githubissues

tylerlittlefield commented 5 years ago

Submitting Author: Tyler Littlefield (@tyluRp)
Repository: https://github.com/tyluRp/exoplanets
Version submitted: 0.0.0.9000 Editor: !--editor-->@maelle<!--end-editor-- Reviewers: @nmonhait, @maelle

Due date for @nmonhait: 2020-08-05 Due date for @maelle: 2021-05-22

Archive: TBD
Version accepted: TBD

Paste the full DESCRIPTION file inside a code block below:

Package: exoplanets
Title: Provides access to NASA's Exoplanet Archive, see <https://exoplanetarchive.ipac.caltech.edu/index.html>
Version: 0.0.0.9000
Authors@R: 
    person(given = "Tyler",
           family = "Littlefield",
           role = c("aut", "cre"),
           email = "tylurp1@gmail.com",
           comment = c(ORCID = "0000-0002-6020-1125"))
Description: The goal of exoplanets is to provide access to NASA's Exoplanet 
  Archive database by supplying a table name or by writing the URL query 
  manually. For more information please read the documentation 
  <https://exoplanetarchive.ipac.caltech.edu/index.html>.
Depends: R (>= 2.10)
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.1.1
Suggests: 
    tibble,
    testthat (>= 2.1.0),
    covr,
    knitr,
    rmarkdown,
    dplyr
Imports: 
    utils
Language: en-US
URL: https://github.com/tyluRp/exoplanets
BugReports: https://github.com/tyluRp/exoplanets/issues
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.):
- [x] data retrieval
- [ ] data extraction
- [ ] database access
- [ ] data munging
- [ ] data deposition
- [ ] reproducibility
- [ ] geospatial data
- [ ] text analysis
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
- The package falls under the data retrieval category because it pulls data from NASA's Exoplanet Archive Database.
Who is the target audience and what are scientific applications of this package?
- Target Audience: astronomers, space enthusiasts, NASA datanauts
- Applications: Planet hunting
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
- None that I am aware of, there is a thread that discusses NASA APIs but I'm not sure if the Exoplanet Archive was ever targeted or implemented in an R package. Additionally there is nasadata which provides access to NASA's EONET API. Same API page but doesn't include the Exoplanet archive as far as I can tell.
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.
- 308

Technical checks

Confirm each of the following by checking the box. This package:

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

Publication options

[x] Do you intend for this package to go on CRAN?
[ ] 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](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*)

[ ] 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](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

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

geanders commented 5 years ago

Editor checks:

[x] Fit: The package meets criteria for fit and overlap
[x] Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
[x] License: The package has a CRAN or OSI accepted license
[x] Repository: The repository link resolves correctly

Editor comments

This looks like a really interesting package, thanks for submitting!

We run a few standard checks on all new packages, using spelling::spell_check_package(), goodpractice::gp(), devtools::spell_check(), and covr::package_coverage().

From spelling::spell_check_package() and devtools::spell_check(): No true spelling errors identified, all looks good for that.

From covr::package_coverage(): 100% coverage, great!

From goodpractice::gp(): There are several lines of code longer than 80 characters that could be shortened.

Here are some small fixes to consider based on those checks:

[ ] Please add a 'Language' field to the 'DESCRIPTION' file (e.g., 'Language = en-US'.
[ ] Try to get the following lines of code under 80 characters: in 'exoplanets.R', lines 37, 42, 45, 62, 65, and 67; in 'test-exoplanets.R', line 12.

For the next step, I'll assign reviewers for the submission. Please let me know if you have any questions during the process!

Reviewers: @h21k, @nmonhait Due date: August 5

tylerlittlefield commented 5 years ago

Awesome!

I've added Language: en-US to the DESCRIPTION file and updated the codemeta.json file.

I have shortened a few of the > 80 lines. A few remain but they are mostly due to a really long API URL. I would prefer to retain that URL instead of paste0()ing or glue()ing it together in chunks. If this is against policy just let me know and I'll make sure nothing exceeds the 80 character limit 😸

geanders commented 5 years ago

@tyluRp : Thanks so much for those updates. I don't think we have hard-and-fast rules for anything in the packages, just guidance. If you have thought about good practice guidelines and have a case where it makes sense to you to make an exception, that seems reasonable.

We now have one reviewer assigned (@h21k) and I am continue to look for the second reviewer.

geanders commented 5 years ago

We now have our second reviewer: @nmonhait. I'm assigning a due date for reviews of August 5.

tylerlittlefield commented 5 years ago

Yay! Excited for feedback, thank you!!

nmonhait commented 5 years ago

Hi @tyluRp , I will need to submit later this week. Sorry for the delay!

tylerlittlefield commented 5 years ago

@nmonhait No problem at all! Take your time 👍

h21k commented 5 years ago

Hi all,

Apologies, I would also need some time to be able to submit (approx around the 20th of August).

Very best wishes,

Frank

On Wed, 7 Aug 2019 at 18:17, Tyler Littlefield notifications@github.com wrote:

@nmonhait https://github.com/nmonhait No problem at all! Take your time 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/309?email_source=notifications&email_token=AGH7LOCQNRY57RKDR6CDGKLQDNX2NA5CNFSM4HPU56IKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD32EJLQ#issuecomment-519324846, or mute the thread https://github.com/notifications/unsubscribe-auth/AGH7LOD6FZIC3D46HEKNX33QDNX2NANCNFSM4HPU56IA .

-- Dr Frank Soboczenski

Personal: https://h21k.github.io NASA Globe: https://www.globe.gov/web/frank.soboczenski/home NASA FDL: http://frontierdevelopmentlab.org/ Twitter: h21k

Latest work on gravitational wave ML models: https://github.com/h21k/GravitationalWavesML

geanders commented 5 years ago

@nmonhait and @h21k : Just wanted to quickly check in and see your status on this review?

nmonhait commented 5 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

[X] 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:

[X] A statement of need clearly stating problems the software is designed to solve and its target audience in README
[X] Installation instructions: for the development version of package and any non-standard dependencies in README
[X] Vignette(s) demonstrating major functionality that runs successfully locally
[X] Function Documentation: for all exported functions in R help
[X] 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- N/A

[ ] 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

[X] Installation: Installation succeeds as documented.
[X] 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

[X] 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

Hi @tyluRp, great work on this package! My comments are below:

README

there is no statement about contributions or code of conduct (unless I am missing this somewhere/if so, this could be more explicit)
I would like to see more about the data and who can use the package/how. You do a great job of this in this issues thread. I imagine if this were the first touchpoint for a user, they would appreciate some additional background -it may be helpful to link to vignettes here

General -there is some code that spans longer than 80 chars -I had LaTex errors, this may be specific to my machine but might be something to look further into -tell me more about this statement - "Finally, there are 3 tables which require an additional parameter in order to access them, these are all time series datasets:", why are these different? -if I were a newcomer, I would need some elaboration on this sentence- "For example, the kelttimeseries table requires either the quarter or a Kepler ID, exo("kelttimeseries&quarter=14"). Note, these datasets are quite large and can take awhile. All of these additional parameter requirements are documented in the documentation."

devtools::check()

0 errors ✔ | 0 warnings ✔ | 0 notes ✔

devtools::test()

OK: 9 Failed: 0 Warnings: 0 Skipped: 0

goodpractice::gp()

✖ avoid long code lines, it is bad for readability. Also, many people prefer editor windows that are about 80 characters wide. Try make your lines shorter than 80 characters

R/exoplanets.R:37:1
R/exoplanets.R:65:1
tests/testthat/test-exoplanets.R:12:1

To summarize I think you did a great job. My main suggestion is to add more detail aimed at the 'first-touchpoint' user. Thorough documentation and vignettes will allow newcomers to utilize the functionality of this package.

tylerlittlefield commented 5 years ago

@nmonhait Thanks a bunch for all the feedback! I will start working on the things you have mentioned. I am curious about the LaTeX errors. If possible, could you elaborate? I do recall adding exo_summary recently and one of the attributes had a >= sign in unicode, this has since been removed but maybe there are some lurking around.

TODO:

[x] Add CONTRIBUTING.md
[x] Add CODE_OF_CONDUCT.md
[x] Add "getting started" vignette
[x] Add a clear and concise explanation of time series datasets and why they are different
[x] Add functionality to address time series datasets, perhaps a function exo_timeseries dedicated to these datasets.
- Three functions have been added (exo_wasp, exo_kelt, exo_kepler) to accommodate time series data per https://github.com/tyluRp/exoplanets/commit/366a9e5bf9b4b949007d0ff75e104b759ff13551

Let me know if I missed anything. Additionally, I recently made a somewhat large change to the package. I've decided to use httr::GET instead of read.csv for the progress messages and to allow better handling of the requests for the future. For some reason, this change has caused the appveyor build to fail. If anyone is using Windows, I'd be interested to see if this package works for you. Finally, if anyone knows why it's failing please reach out.

Thanks again for the feedback 👍

geanders commented 5 years ago

Thank you so much for your review, @nmonhait! @h21k, do you have an update on when you would be able to add your review?

@tyluRp : Regarding your Windows question, I'm using a Mac, so I can't check. Perhaps @nmonhait or @h21k is using Windows and could check. Also, have you tried out the package through http://win-builder.r-project.org/? I'm not sure if that's going to provide more details for you than the Appveyor output, but it might be worth trying if you haven't yet. If you're using the devtools package, the build_win and check_win functions both interface with the winbuilder site.

tylerlittlefield commented 5 years ago

@geanders Thanks for the advice. I've just tried this out on a windows machine and I am getting the same error:

Error in curl::curl_fetch_memory(url, handle = handle) : 
  Failure when receiving data from the peer

I will use the devtools functions to see if this provides any additional information.

UPDATE: Wanted to provide an update for any Windows users. I filed an issue for the curl::curl_fetch_memory error here https://github.com/jeroen/curl/issues/206. Some interesting responses.

I've decided to add two utility functions to the package for handling requests differently depending on the operating system (windows, or anything else), you can see the additions here https://github.com/tyluRp/exoplanets/commit/6b93fb231cb408936be0d03fea42bb845b12e0c5. This has fixed the appveyor build.

tylerlittlefield commented 5 years ago

Hi @nmonhait, thanks again for your feedback. I've added a vignette with a walk through on how to use the package and access data from the archive. I've also added additional helper functions for the time series datasets, the intro vignette covers these new functions as well.

Code of conduct and contributing guidelines have been added. When you have a moment, please let me know your thoughts.

h21k commented 5 years ago

Hi all,

apologies for the delay. I should have mine ready very soon.

Best wishes,

Frank

On Sun, 3 Nov 2019 at 04:01, Tyler Littlefield notifications@github.com wrote:

Hi @nmonhait https://github.com/nmonhait, thanks again for your feedback. I've added a vignette with a walk through on how to use the package and access data from the archive. I've also added additional helper functions for the time series datasets, the intro vignette covers these new functions as well.

Code of conduct and contributing guidelines have been added. When you have a moment, please let me know your thoughts.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/309?email_source=notifications&email_token=AGH7LOEYMDEZN7QNK26VURDQRZEIHA5CNFSM4HPU56IKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC5KNCA#issuecomment-549103240, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGH7LOEQH5LK2IUO7EY62DLQRZEIHANCNFSM4HPU56IA .

geanders commented 5 years ago

Thank you for the update, @h21k! Looking forward to your review!

geanders commented 4 years ago

@h21k : I just wanted to check in and see if you will be able to have your review in soon? Thanks again so much for reviewing this package!

geanders commented 4 years ago

@h21k: I wanted to check and see if you will still be able to review this package?

noamross commented 4 years ago

⚠️⚠️⚠️⚠️⚠️

In the interest of reducing load on reviewers and editors as we manage the COVID-19 crisis, rOpenSci is temporarily pausing new submissions for software peer review for 30 days (and possibly longer). Please check back here again after 17 April for updates.

In this period new submissions will not be handled, nor new reviewers assigned. Reviews and responses to reviews will be handled on a 'best effort' basis, but no follow-up reminders will be sent.

Other rOpenSci community activities continue. We express our continued great appreciation for the work of our authors and reviewers. Stay healthy and take care of one other.

The rOpenSci Editorial Board

⚠️⚠️⚠️⚠️⚠️

annakrystalli commented 4 years ago

⚠️⚠️⚠️⚠️⚠️ In the interest of reducing load on reviewers and editors as we manage the COVID-19 crisis, rOpenSci new submissions for software peer review are paused.

In this period new submissions will not be handled, nor new reviewers assigned. Reviews and responses to reviews will be handled on a 'best effort' basis, but no follow-up reminders will be sent. Other rOpenSci community activities continue.

Please check back here again after 25 May when we will be announcing plans to slowly start back up.

We express our continued great appreciation for the work of our authors and reviewers. Stay healthy and take care of one other.

The rOpenSci Editorial Board ⚠️⚠️⚠️⚠️⚠️

annakrystalli commented 4 years ago

⚠️⚠️⚠️⚠️⚠️ In the interest of reducing load on reviewers and editors as we manage the COVID-19 crisis, rOpenSci new submissions for software peer review are paused.

In this period new submissions will not be handled, nor new reviewers assigned. Reviews and responses to reviews will be handled on a 'best effort' basis, but no follow-up reminders will be sent. Other rOpenSci community activities continue.

Please check back here again after 25 May when we will be announcing plans to slowly start back up.

We express our continued great appreciation for the work of our authors and reviewers. Stay healthy and take care of one other.

The rOpenSci Editorial Board ⚠️⚠️⚠️⚠️⚠️

geanders commented 4 years ago

Hi, @h21k! We had paused some parts of our review process due to COVID, but now have gone back to our regular process. I wanted to check in and see if you are still able to review this package, or if I should search for a different reviewer to take your place? Thanks very much for any feedback you can give!

tylerlittlefield commented 4 years ago

Hi @geanders and @h21k, I have someone (@mathidachuk) who is interested in reviewing the package if needed. Please let us know!

h21k commented 4 years ago

Hi Tyler,

thank you for the info - I should be able to pick this up by end of the month. So happy to review.

Best wishes,

Frank

On Wed, 22 Jul 2020 at 21:59, Tyler Littlefield notifications@github.com wrote:

Hi @geanders https://github.com/geanders and @h21k https://github.com/h21k, I have someone (@mathidachuk https://github.com/mathidachuk) who is interested in reviewing the package if needed. Please let us know!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/309#issuecomment-662693489, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGH7LOHPRIPJOYDSL77TZSLR45HMRANCNFSM4HPU56IA .

-- Dr Frank Soboczenski

Researcher (AI) in Health Informatics - King's College London STEM Scientist for NASA's & NOAA's GLOBE Program Orion Labs Machine Learning Scientist

Personal: https://h21k.github.io NASA Globe: https://www.globe.gov/web/frank.soboczenski/home Huggingface models: https://huggingface.co/kenobi US Navy TLPD project:

tylerlittlefield commented 4 years ago

Thanks @h21k, appreciate it!

@geanders is it possible to have more than 2 reviewers? @mathidachuk is a colleague of mine who has started participating in open source work. If she can contribute to this software review, I think it would be a good opportunity for her.

geanders commented 4 years ago

We would love to get @mathidachuk involved as an ROpenScie reviewer! For this specific package, we do have a conflict of interest policy. Let's start by having the two of you take a look at the conflict of interest policies. I think if you are colleagues at the same institution, then @mathidachuk might be conflicted out from this review. However, we have some tips on becoming a reviewer, which includes a link to a form for joining a database of reviewers that we use a lot to help find reviewers for specific packages. This would give @mathidachuk the chance to review other packages, even if there is a conflict of interest for this one.

geanders commented 4 years ago

@h21k : Thanks so much, and we look forward to your review!

tylerlittlefield commented 4 years ago

@geanders Thanks for the links, good to know!

maelle commented 4 years ago

:wave: @h21k, are you still able to review the package? If so here's the link to our reviewers guide and review template.

maelle commented 3 years ago

:wave: @tyluRp I'll handle this submission from now on. @geanders will be the second reviewer. Thanks for your patience!

tylerlittlefield commented 3 years ago

@maelle Sweet, appreciate it! I'll keep an eye out.

maelle commented 3 years ago

@ropensci-review-bot add @geanders to reviewers

ropensci-review-bot commented 3 years ago

@geanders added to the reviewers list. Review due date is 2021-03-17. Thanks @geanders for accepting to review! Please refer to our reviewer guide.

maelle commented 3 years ago

@geanders friendly reminder about your review :slightly_smiling_face:

maelle commented 3 years ago

@ropensci-review-bot add @maelle to reviewers

ropensci-review-bot commented 3 years ago

@maelle added to the reviewers list. Review due date is 2021-05-22. Thanks @maelle for accepting to review! Please refer to our reviewer guide.

maelle commented 3 years ago

@ropensci-review-bot remove @geanders from reviewers

ropensci-review-bot commented 3 years ago

@geanders removed from the reviewers list!

maelle commented 3 years ago

@tyluRp about to start reviewing, sorry for the delay and thanks for your patience!

maelle commented 3 years ago

Thanks again for your submission and patience @tyluRp! :warning: exo_columns() fails because the data changed upstream. Therefore the tests and vignette building fail. I've made some other comments. Some of them are suggestions, the most important comments are about documentation and improving testing. Happy to discuss!

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.
[x] 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:

[x] A statement of need clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s) demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions
[ ] Examples (that run successfully locally) for all exported functions
[ ] 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

[x] 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.
[x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing: 1

[x] 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

This is a nice package for people who'll need to use this data in R! :dizzy:

CRAN submission

I see traces of a CRAN submission, but the packages isn't on CRAN yet, what happened? In any case, now it'd make sense to wait until after the review changes to re-submit. Also note that there is no obligation to submit the package to CRAN.

Documentation

You could rename the vignette to "explanets.Rmd" for having it automatically listed under "Get started" in the navbar by pkgdown.
I saw two "click here" links, which is better avoided in particular for accessibility.
- https://github.com/tyluRp/exoplanets/blob/6bff47e1c3693ee093e86b658eb3a1efd4c721d4/README.Rmd#L139
- https://github.com/tyluRp/exoplanets/blob/6bff47e1c3693ee093e86b658eb3a1efd4c721d4/R/exo_summary.R#L4
In the documentation of some functions you write e.g. "See the KELT page", it would be nice to have links to such pages.
In the documentation of exo_tables() it'd be nice to have an example as it'd mean the table would be present in the pkgdown website.
When documenting exo_raw() could you add some explanations of the example?
In the docs of e.g. exo(), for the parameter cols you could add how to know what columns are default, what columns are available (simply linking to exo_columns() docs I suspect).
The quiet argument, and maybe others, is repeatedly documented. You could use roxygen2's parameter inheritance or templates to avoid this.
The README image is quite cool!
- Could you please add an alternative text for it?
- Could you explain more about the image and how you created it using the package, perhaps in a vignette linked from the README?
I'd recommend precomputing the vignette (especially if you submit your package to CRAN).

Contributing information

There is no CONTRIBUTING.md at the moment. We have some basic guidance around this in the dev guide, that I will update in the upcoming weeks based on our recent community call.

When time comes to transfer your repository to the ropensci organization, we'll ask you remove the Code of Conduct file as rOpenSci Code of Conduct will apply to your package.

Code

Why not use httr::RETRY() (or httr::GET()) for downloading the data? You could then stop for status.
Would it make sense to try and add a progress bar for downloads? As they are failing for now, I do not know how long they take, though.
For formatting the messages e.g. the ones with URLs you might be interested in the cli package.
Why do you memoise the functions in .onLoad()? Genuinely curious, as we use a different strategy in e.g. opencage.

Testing

Tests currently fail, but I also find them a bit light at the moment.

You could test more API calls. I'd advise to mock them somehow, using e.g. vcr or httptest. We updated the HTTP testing in R book last year. I'm happy to answer any question on the topic.
You could test the user-friendly error messages, using testthat snapshot testing.

Continuous integration

I see you use GitHub Actions, just a note that tidyverse repos seem to be switching towards pak-based workflows (whose names end with -pak) instead of remotes-based workflows.
To catch a problem as the one in exo_columns() you could add a regularly scheduled workflow. Note that in GitHub Actions, after 60 days without repo activity, scheduled workflows are deactivated.

Thanks again for your submission & work on this neat&useful package! :ringed_planet:

tylerlittlefield commented 3 years ago

Thanks for the review!

Regarding CRAN, there was an odd bug accessing the API from a windows machine in the check so I could never get the build to pass.
Regarding httr, I actually used this from the beginning but eventually removed it thinking this might somehow resolve the CRAN issue, https://github.com/ropensci/software-review/issues/309#issuecomment-534840574
When I had httr progress bar was a nice to have, I made it so you could turn it on/off but had since removed due to point above.
Regarding memoise, I just wanted to check it out and see how caching results work. It seems like it could be nice if someone is hitting the api more than once with the same query.

The 60 day scheduled check sounds really nice, especially for a package that depends on an API. I'll review this feedback over the weekend and start updating things. Thanks!

tylerlittlefield commented 3 years ago

@maelle Okay! So I've gone ahead an basically rewrote the entire package due to API changes. AFAIK, the function used to get column information broke last Friday so basically everything broke as you mentioned. This rewrite uses the new "TAP" service offered by the exoplanet archive. I've decided to support the new interface for now as it looks like tables from the old interface are planned to be migrated to "TAP" in the future. If things settle down, I may decide to support the old API.

Summary of changes:

Entire rewrite, supporting new "TAP" service instead of old "API" service
Precomputed/updated vignette
Use of httr for requests instead of read.csv
Progress bars
Updated table information dataset (with example)
Added contributing guidelines
Update workflows to use -pak based
Added alternative text to README
Removed "click here" style links

Pending items:

~~Tests! I've got to read up on this and add some tests for the actually requests.~~
Community guidelines?
Code of conduct

Anyway, this should be good enough to actually see the package in action. I will post an update once I'm happy with the tests. Please let me know if you have any questions.

tylerlittlefield commented 3 years ago

Alright, I think I'm done, I've added:

Tests with httptest
Weekly r cmd check, basically the check-pak.yml workflow but triggered by cron instead of commits (I assume this is how it's done, never tried before!)

Also wanted to say thanks a bunch, this feedback has taught me quite a bit, specifically:

How awesome httptest is, I'm hoping this makes my life easier when trying to submit this to CRAN.
Precompiled vignettes! Ditto on point 1, I hope it makes submission easier.
Reoccurring workflows!

Lastly, going back to the memoise question. I looked at what the link you provided and I am not quite getting what is off with my implementation, any further advice on how to improve that part would be appreciated!

maelle commented 3 years ago

Thanks @tyluRp for all your work on this! :rocket: :sparkles:

My answers

Please summarize changes in NEWS.md especially as the changes were quite big. :slightly_smiling_face:
I feel less bad about the delay now that I know it means I reviewed the package right after the API calls. :grin: (seriously, thanks again for your patience!)
Regarding CRAN it's good that now your package has pre-computed vignettes, not-run examples and mocked tests as it means no API call will be made from CRAN. :grin:
Regarding memoise, I realize .onLoad() is actually used in opencage so my comment was not valid.
A very small suggested tweak, if you run desc::desc_normalize() dependencies will be alphabetically ordered and Imports will come before Suggests which is more usual.
Regarding your wondering whether things will settle down with the API, do you need to contact the API maintainers?
The URL in https://github.com/tyluRp/exoplanets/blob/577260b3ce89f1e43e7bf781ffaed6ace1b9222a/R/data.R#L10 needs to be wrapped in \url{} to be clickable on the pkgdown website in particular.
Wouldn't it make sense to use httr::RETRY() instead of httr::GET()?
In the scheduled GitHub Actions run you need to make sure httptest is turned off (at least for one of the builds) as otherwise you can't catch errors due to changes in the API. I don't remember how to turn off httptest, whether you need to delete mock files or whether you can add some sort of switch based on an environmental variable.
In the vignette when re-recreating the plot why not add a few sentences about why to plot these things and why to label "TRAPPIST-1 e"? I.e. providing some context for the use case.

Thanks again!

maelle commented 3 years ago

One last question actually, why keep the random aspect of the tests and not add tests corresponding to the same use cases as the examples in the manual?

tylerlittlefield commented 3 years ago

NEWS.md updated.
desc::desc_noarmalize() used.
~~I do not think I need to reach out the maintainers at this time. Will just need to review and monitor their changelog from time to time.~~ Nevermind, I was curious and asked if their plan is to eventually migrate everything from the old API to their new TAP service and the answer is yes. So I think I will continue to not support API as it will be deprecated in the future.
Wrapped link in \url{}.
Using httr::RETRY().
Removed scheduled github action, will take a look at this at a later time. Note to self, I'll want to take a look at https://github.com/nealrichardson/httptest/issues/40.
Added some text on why "TRAPPIST-1 e" is labeled.
Added tests for most of the examples (excluded the one that requests the default ps table which is huge). As for why they are random, I was just trying to cover things outside the examples, like expected errors, different parameters/scenarios, etc.

Please let me know if theres anything else I missed!

tylerlittlefield commented 3 years ago

By the way, the desc package is pretty cool and when I used it, it showed the following for the authors part of the DESCRIPTION:

Authors@R (parsed):
  * Tyler Littlefield <tylerlittlefield@hey.com> [aut, cre] (<https://orcid.org/0000-0002-6020-1125>)

But devtools::document() was not liking it. Am I supposed to be able to record authors in the DESCRIPTION file this way?

maelle commented 3 years ago

Some comments (probably last comments). Thanks for your work!

I've opened a PR to fix the link of the GitHub Actions badge.
Instead of having to force silence of the package why not add a verbose option / argument? Having an option for verbosity is e.g. the approach in usethis and gargle.
When you use "data.frame" %in% class in expectations in tests it'd be more elegant to use expect_type() instead.
Instead of expect_error() if you use expect_snapshot_error() your test will be more precise.
For these two last tips maybe you need to switch to testthat 3d edition in which case you can refer to its release post.
Reg desc what it prints in the console is not exactly what needs to go in the file, but when you run desc::desc_normalize() desc itself edits the DESCRIPTION file. Not sure if I understood the question right though!

tylerlittlefield commented 3 years ago

Thanks for the PR! I've added you to the description file as a contributer/reviewer.
A quiet option sounds nice but I'm also using the quiet function in the testthat script to avoid this message "Using redact.R in 'exoplanets'" message that seems to appear when testing. I want the test log to be as clean as possible.
Thanks for the suggestion on expect_type, I went with expect_s3_class because typeof on a dataframe is list and I'll need to be able to distinguish lists vs dataframes.
Using expect_snapshot_error, thanks for the suggestion!

Please let me know if there is anything else :)

maelle commented 3 years ago

@ropensci-review-bot approve

ropensci / software-review

Submission: exoplanets #309

Scope

308

Technical checks

Publication options

Code of conduct

Editor checks:

Editor comments

Package Review

Documentation

For packages co-submitting to JOSS- N/A

Functionality

Final approval (post-review)

Review Comments

Package Review

Documentation

Functionality

Review Comments

CRAN submission

Documentation

Contributing information

Code

Testing

Continuous integration