Rpolyhedra - Githubissues

kenarab commented 6 years ago

Submitting Author: Alejandro Baranek" (@kenarab) Repository: https://github.com/qbotics/Rpolyhedra Version submitted: 0.1.0 Editor: @noamross Reviewer 1: @yulijia Reviewer 2: @schloerke Archive: N/A Version accepted: 0.4.1

Summary

What does this package do? (explain in 50 words or less):

A 142 polyhedra Database scraped from PHD as R6 objects and RGL visualizing capabilities. The PHD format was created to describe the geometric polyhedron definitions derived mathematically by Andrew Hume and by the Kaleido program of Zvi Har'El.

Paste the full DESCRIPTION file inside a code block below:

Package: Rpolyhedra
Type: Package
Title: Polyhedra Database
Version: 0.1.0
Authors@R: c(
    person("Alejandro", "Baranek",,"abaranek@dc.uba.ar", c("aut","com","cre", "cph")),
    person("Leonardo","Belen",,"leobelen@gmail.com", c("aut","com","cph"))
    )
Maintainer: Alejandro Baranek <abaranek@dc.uba.ar>
Description: A 142 polyhedra database scraped from PHD files <http://paulbourke.net/dataformats/phd/> as R6 objects and 'rgl' visualizing capabilities. The PHD format was created to describe the geometric polyhedra definitions derived mathematically <http://www.netlib.org/polyhedra/> by Andrew Hume and by the Kaleido program of Zvi Har'El.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.0.1
Suggests: knitr, rmarkdown,
    covr
VignetteBuilder: knitr
Depends: R (>= 3.0.0)
Imports: futile.logger,
         rgl,
         stringr,
         R6,
         testthat
Collate:
  'polyhedra-lib.R'
  'zzz.R'
BugReports: https://github.com/qbotics/Rpolyhedra/issues

URL for the package (the development repository, not a stylized html page): https://github.com/qbotics/Rpolyhedra
Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):

[e.g., "data extraction, because the package parses a scientific data file format"]

data retrieval, because includes the code for downloading the phd files data extraction, because the package parses a scientific data file format database access, because it gives access to PHD polyhedra database

Other categories: legacy sourcecode: as poly is an opensource inciative (from '80s) than no longer compiles, the project gives access to original poly outputs.

Who is the target audience?
- Scientists and general public who wants to explore polyhedra through RGL.
- Scientists and general public who wants to study polyhedra's properties and make taxonomies
- Scientists and general public who wants to make models over polyhedra definitions
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?

RGL includes 5 regular solids. Rpolyhedra provides version of the 5 regular solids and 137 more polyhedra.

Requirements

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.
[ ] has continuous integration, including reporting of test coverage, using services such as Travis CI, Coeveralls and/or CodeCov.
[X] I agree to abide by ROpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

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

Detail

[X] Does R CMD check (or devtools::check()) succeed? Paste and describe any errors or warnings:
[X] Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
Delete the file inst/extdata/polyhedra.RDS for scraping and regenerating it.
Compile the package with a small number of polyhedra first:
```
.polyhedra <- scrapePolyhedra(max.quant = 5, test = FALSE)
```
We have to find a way for accelerate the scraping of the whole database. Maybe a reviewer know a tweak.

sckott commented 6 years ago

thanks for your submission @kenarab

Please change the URL to https://github.com/qbotics/Rpolyhedra/ instead of https://github.com/cran/Rpolyhedra/

We need some clarification to assess fit

What are the scientific uses of these data? You say scientists are a target audience, but what kind? What would they do with these data?
I know little about this area. Are there alternative ways to specify polyhedra? If so, what is most widely used?

kenarab commented 6 years ago

Thank you @sckott.

I corrected the URL, and extended the users with this paragraph:

Scientists and general public who wants to explore polyhedra through RGL.
Scientists and general public who wants to study polyhedra's properties and make taxonomies
Scientists and general public who wants to make models over polyhedra definitions

About

I know little about this area. Are there alternative ways to specify polyhedra? If so, what is most widely used?

PHD is a legacy open source specification for polyhedra. As we found online that database with solid definition we chose to use it for further research over polyhedra. For specifying polyhedra solids there is only needed to describe vertex and edges (and the definition is not exclusive). But there are different properties the specialists uses: Schaffi symbols, symmetry/rotation groups, etc.

For example, in Wikipedia there are a lot of information. https://en.wikipedia.org/wiki/Icosahedron The visualizations of wikipedia are generally made in POV-ray. There is currently no way for accessing a large database of solid (and net) models of polyhedra in R. That's the target of Rpolyhedra.

Rpolyhedra includes algorithms for triangulating polygon faces for producing RGL Models based only in trangles (tmesh).

We didn't work yetwith polyhedra formulation and generation in Rpolyhedra (Is not really our objective). We only give access to other people work, It's a compilation work. There are private projects with polyhedra databases but RGL includes only models for 5 regular solids as RGL models. That's why we think Rpolyhedra could be useful for R community.

sckott commented 6 years ago

Thank you @kenarab

sckott commented 6 years ago

hi @kenarab thx for your submission - @noamross will be handling editor

noamross commented 6 years ago

Editor checks:

[x] 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.
[x] License: The package has a CRAN or OSI accepted license
[x] 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

Hi @kenarab, thanks for your submission, I think this is a really neat package. I can think of a few people who would be interested already who I might ask to review. A few things before we can assign reviewers, though:

We require that packages have continuous integration (typically Travis-CI), and code coverage reporting (typically codecov or coveralls). Looking at your package tests, I see you are testing the scraping functions but not necessarily what goes on in the R6. We don't require 100% code coverage, but we'd like coverage of the major functionality. As far as I can tell there are three major functionalities: scraping the data from netlib, parsing the PHD format, and providing R6 objects with polyhedral data. All three should have associated tests. You can use skip_on_cran() or skip_on_travis() if long-running scraping calls or RGL calls will break that. Let us know if you'd like some pointers to setting this infrastructure set up.
It looks like you have some debugging infrastructure hard-coded into the package right now via the futile.logger calls. This breaks other testing tools and also results in unneeded/confusing messages bring printed out to the user. This should be removed. You might keep it on a development branch.
I note that the code doesn't run in your vignette because there isn't a library(rgl) call. You also should consider using the WebGL capability in the rgl package so that you can make a vignette that compiles and shows.
Documentation is very sparse - most functions have a single sentence. We try to aim documentation to be sufficient for familiars unfamiliar with the original data source. R6 classes can be a bit difficult to document, but docs should detail the arguments and formats of the member functions. We want documentation to be as complete as possible at submission so reviewers can give feedback on it.

kenarab commented 6 years ago

Thank you Noam.

Thank you very much for your sharp observations. We are already working on the recommendations and next week will push a new release including the proper changes.

Best regards, Alejandro.

2017-11-16 18:18 GMT-03:00 Noam Ross notifications@github.com:

Editor checks:

Fit: The package meets criteria for fit http://policies.md#fit and overlap http://policies.md#fit

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

Hi @kenarab https://github.com/kenarab, thanks for your submission, I think this is a really neat package. I can think of a few people who would be interested already who I might ask to review. A few things before we can assign reviewers, though:

We require that packages have continuous integration (typically Travis-CI), and code coverage reporting (typically codecov or coveralls). Looking at your package tests, I see you are testing the scraping functions but not necessarily what goes on in the R6. We don't require 100% code coverage, but we'd like coverage of the major functionality. As far as I can tell there are three major functionalities: scraping the data from netlib, parsing the PHD format, and providing R6 objects with polyhedral data. All three should have associated tests. You can use skip_on_cran() or skip_on_travis() if long-running scraping calls or RGL calls will break that. Let us know if you'd like some pointers to setting this infrastructure set up.

It looks like you have some debugging infrastructure hard-coded into the package right now via the futile.logger calls in you .onLoad(). This breaks other testing tools and also results in unneeded/confusing messages bring printed out to the user. This should be removed. You might keep it on a development branch.

I note that the code doesn't run in your vignette because there isn't a library(rgl) call. You also should consider using the WebGL capability https://cran.r-project.org/web/packages/rgl/vignettes/WebGL.html in the rgl package so that you can make a vignette that compiles and shows.

Documentation is very sparse - most functions have a single sentence. We try to aim documentation to be sufficient for familiars unfamiliar with the original data source. R6 classes can be a bit difficult to document, but docs should detail the arguments and formats of the member functions. We want documentation to be as complete as possible at submission so reviewers can give feedback on it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/157#issuecomment-345065778, or mute the thread https://github.com/notifications/unsubscribe-auth/ACzA1qJXuxcKBb4hqWp0yLkuRol1w9XNks5s3Ka4gaJpZM4QYpu6 .

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

noamross commented 6 years ago

Any update on this, @kenarab?

kenarab commented 6 years ago

Hello Noam:

I'm sorry but we have been too much busy with the end of the year. I hope w will be making a submission on 2017 :)

Happy new year for you.

Best regards.

2017-12-08 23:05 GMT-03:00 Noam Ross notifications@github.com:

Any update on this, @kenarab https://github.com/kenarab?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/157#issuecomment-350415854, or mute the thread https://github.com/notifications/unsubscribe-auth/ACzA1v1a-BPE_UBJXZ7mh48Yh-DLAFOFks5s-er_gaJpZM4QYpu6 .

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

noamross commented 6 years ago

Hi @kenarab! Just checking to see if you have a time frame for following up on this.

kenarab commented 6 years ago

Hello Noam:

We are up to push, but couldn’t made the timeframe to concentrate on the release. We hope will do it in the next days.

Best regards, Ale.

Enviado desde el móvil

El 22 feb. 2018, a la(s) 13:33, Noam Ross notifications@github.com escribió:

Hi @kenarab! Just checking to see if you have a time frame for following up on this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

kenarab commented 6 years ago

Dear Noam:

We worked hard this week on Rpolyhedra. We are including another polyhedra database with +700 polyhedra. We already working with TRAVIS. If everything ok we setup tomorrow the V0.2.0 release including both databases.

All the best, Ale.

2018-02-22 19:32 GMT-03:00 alejandro baranek alejandrobaranek@gmail.com:

Hello Noam:

We are up to push, but couldn’t made the timeframe to concentrate on the release. We hope will do it in the next days.

Best regards, Ale.

Enviado desde el móvil

El 22 feb. 2018, a la(s) 13:33, Noam Ross notifications@github.com escribió:

Hi @kenarab https://github.com/kenarab! Just checking to see if you have a time frame for following up on this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/157#issuecomment-367739117, or mute the thread https://github.com/notifications/unsubscribe-auth/ACzA1leA-EvKyl13L3-fYYVqMIvJrScZks5tXZbpgaJpZM4QYpu6 .

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

noamross commented 6 years ago

Great, Ale. Let me know when you want me to look again so I can assign reviewers. I'd suggest running goodpractice::gp before you do as that's what we run to check packages before sending out to review.

kenarab commented 6 years ago

Noam:

We worked hard including the new source. We have persisting small issues. For example, Travis fail because it takes a long time to scrape all the database. We will make a prescraped polyhedron list for the simpler polyhedron. And (without testing) bigger poyhedra will be loaded on demand.

But the problem that's stopped us for publication is goodpractice::gp gives a strange error we cannot reproduce with our tests. I hope in the next days we'll be able to make the submission.

By the way, this versions includes some exciting feature like a reproducible research compliant scraping scheduler (For problems with scraping specific models don't stop us to publicate), polyhedra slicing (For big polyhedra, like geodesic ones), and exporting Vertices and Edges models, and of Course, original Faces models.

We are making a parallel project for making a simple Shinny interface for Rpolyhedra. If there is somewhere to host it we will be able to reach broader users, like people who doesn't use R but want to use polyhedrons. Architect, desgingers and artists, for example.

Best regards, Ale.

2018-03-01 18:26 GMT-03:00 Noam Ross notifications@github.com:

Great, Ale. Let me know when you want me to look again so I can assign reviewers. I'd suggest running goodpractice::gp https://github.com/MangoTheCat/goodpractice before you do as that's what we run to check packages before sending out to review.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/157#issuecomment-369736166, or mute the thread https://github.com/notifications/unsubscribe-auth/ACzA1s_cJA2imDk-JSAemkpGrzN7mYneks5taGdogaJpZM4QYpu6 .

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

kenarab commented 6 years ago

Noam:

We just pushed release V0.2.0. https://github.com/qbotics/Rpolyhedra/releases/tag/v0.2.0

We have problems with goodpractice we didn't have time to attend yet.

We are uploading this release in CRAN while waiting the review. After the reviewers work we hope can make a new release directly from Ropensci.

Thank you very much for your advisor.

Best regards, Ale.

2018-03-08 16:45 GMT-03:00 alejandro baranek alejandrobaranek@gmail.com:

Noam:

We worked hard including the new source. We have persisting small issues. For example, Travis fail because it takes a long time to scrape all the database. We will make a prescraped polyhedron list for the simpler polyhedron. And (without testing) bigger poyhedra will be loaded on demand.

But the problem that's stopped us for publication is goodpractice::gp gives a strange error we cannot reproduce with our tests. I hope in the next days we'll be able to make the submission.

By the way, this versions includes some exciting feature like a reproducible research compliant scraping scheduler (For problems with scraping specific models don't stop us to publicate), polyhedra slicing (For big polyhedra, like geodesic ones), and exporting Vertices and Edges models, and of Course, original Faces models.

We are making a parallel project for making a simple Shinny interface for Rpolyhedra. If there is somewhere to host it we will be able to reach broader users, like people who doesn't use R but want to use polyhedrons. Architect, desgingers and artists, for example.

Best regards, Ale.

2018-03-01 18:26 GMT-03:00 Noam Ross notifications@github.com:

Great, Ale. Let me know when you want me to look again so I can assign reviewers. I'd suggest running goodpractice::gp https://github.com/MangoTheCat/goodpractice before you do as that's what we run to check packages before sending out to review.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/157#issuecomment-369736166, or mute the thread https://github.com/notifications/unsubscribe-auth/ACzA1s_cJA2imDk-JSAemkpGrzN7mYneks5taGdogaJpZM4QYpu6 .

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

kenarab commented 6 years ago

Noam:

We made a new release https://github.com/qbotics/Rpolyhedra/releases/tag/v0.2.1

because of CRAN requested corrections as minor changes in Vignette. Please wait for the approving confirmation of this release approved in CRAN I will do, I hope, in the next hour.

2018-03-13 14:27 GMT-03:00 alejandro baranek alejandrobaranek@gmail.com:

Noam:

We just pushed release V0.2.0. https://github.com/qbotics/Rpolyhedra/releases/tag/v0.2.0

We have problems with goodpractice we didn't have time to attend yet.

We are uploading this release in CRAN while waiting the review. After the reviewers work we hope can make a new release directly from Ropensci.

Thank you very much for your advisor.

Best regards, Ale.

2018-03-08 16:45 GMT-03:00 alejandro baranek alejandrobaranek@gmail.com:

Noam:

We worked hard including the new source. We have persisting small issues. For example, Travis fail because it takes a long time to scrape all the database. We will make a prescraped polyhedron list for the simpler polyhedron. And (without testing) bigger poyhedra will be loaded on demand.

But the problem that's stopped us for publication is goodpractice::gp gives a strange error we cannot reproduce with our tests. I hope in the next days we'll be able to make the submission.

By the way, this versions includes some exciting feature like a reproducible research compliant scraping scheduler (For problems with scraping specific models don't stop us to publicate), polyhedra slicing (For big polyhedra, like geodesic ones), and exporting Vertices and Edges models, and of Course, original Faces models.

We are making a parallel project for making a simple Shinny interface for Rpolyhedra. If there is somewhere to host it we will be able to reach broader users, like people who doesn't use R but want to use polyhedrons. Architect, desgingers and artists, for example.

Best regards, Ale.

2018-03-01 18:26 GMT-03:00 Noam Ross notifications@github.com:

Great, Ale. Let me know when you want me to look again so I can assign reviewers. I'd suggest running goodpractice::gp https://github.com/MangoTheCat/goodpractice before you do as that's what we run to check packages before sending out to review.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/157#issuecomment-369736166, or mute the thread https://github.com/notifications/unsubscribe-auth/ACzA1s_cJA2imDk-JSAemkpGrzN7mYneks5taGdogaJpZM4QYpu6 .

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

kenarab commented 6 years ago

Already in CRAN. We wait for your review, in the next days we will try to debug Goodpractice seemly bug.

Enviado desde el móvil

El 13 mar. 2018, a la(s) 16:54, alejandro baranek alejandrobaranek@gmail.com escribió:

Noam:

We made a new release https://github.com/qbotics/Rpolyhedra/releases/tag/v0.2.1

because of CRAN requested corrections as minor changes in Vignette. Please wait for the approving confirmation of this release approved in CRAN I will do, I hope, in the next hour.

2018-03-13 14:27 GMT-03:00 alejandro baranek alejandrobaranek@gmail.com:

Noam:

We just pushed release V0.2.0. https://github.com/qbotics/Rpolyhedra/releases/tag/v0.2.0

We have problems with goodpractice we didn't have time to attend yet.

We are uploading this release in CRAN while waiting the review. After the reviewers work we hope can make a new release directly from Ropensci.

Thank you very much for your advisor.

Best regards, Ale.

2018-03-08 16:45 GMT-03:00 alejandro baranek alejandrobaranek@gmail.com:

Noam:

We worked hard including the new source. We have persisting small issues. For example, Travis fail because it takes a long time to scrape all the database. We will make a prescraped polyhedron list for the simpler polyhedron. And (without testing) bigger poyhedra will be loaded on demand.

But the problem that's stopped us for publication is goodpractice::gp gives a strange error we cannot reproduce with our tests. I hope in the next days we'll be able to make the submission.

By the way, this versions includes some exciting feature like a reproducible research compliant scraping scheduler (For problems with scraping specific models don't stop us to publicate), polyhedra slicing (For big polyhedra, like geodesic ones), and exporting Vertices and Edges models, and of Course, original Faces models.

We are making a parallel project for making a simple Shinny interface for Rpolyhedra. If there is somewhere to host it we will be able to reach broader users, like people who doesn't use R but want to use polyhedrons. Architect, desgingers and artists, for example.

Best regards, Ale.

2018-03-01 18:26 GMT-03:00 Noam Ross notifications@github.com:

Great, Ale. Let me know when you want me to look again so I can assign reviewers. I'd suggest running goodpractice::gp before you do as that's what we run to check packages before sending out to review.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

-- alejandro baranek @ken4rab qbotics | surferinvaders | algebraic-soundscapes | surfer-shuffle

-- alejandro baranek @ken4rab qbotics | surferinvaders | algebraic-soundscapes | surfer-shuffle

-- alejandro baranek @ken4rab qbotics | surferinvaders | algebraic-soundscapes | surfer-shuffle

noamross commented 6 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
[ ] 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

Glad we're getting this going again!

You must include a paper.md so our reviewers can review it on behalf of JOSS if you want joint JOSS publication. This does need to happen before reviewers are assigned, but I will go ahead and look for reviewers and have them start once you have the paper in place.

I note that while you have CI set up, you don't have Travis-CI or Codecov badges in your package README. Please add these - they make it easier to see package status at a glance. Also, you may now add the ROpenSci status badge:

[![](https://badges.ropensci.org/157_status.svg)](https://github.com/ropensci/onboarding/issues/157)

It appears the goodpractice::gp() bug is actually in the covr package. For some covr::package_coverage() fails under the the first test, even though your tests pass with flying colors. I suggest you submit a bug report to covr. Otherwise, Here are goodpractice::gp() checks without using the covr checks. Nothing serious here - please fix the small stuff. Reviewers should consider whether changes to code style or package imports are necessary.

Regarding the large file issue: if you have a lot of large polyhedra you would like to serve to users but cannot get on CRAN, may I suggest adding an after-the-fact download function like download_polyhedra(). This is a strategy used by several others; the recently accepted rtika packages uses this approach to deliver a large .jar after installation. Your decision on this can wait until after reviews are in an reviewers have weighed in.

── GP Rpolyhedra ─────────────────────────────────────

It is good practice to

  ✖ 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.
  ✖ 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/db-lib.R:32:1
    R/db-lib.R:67:1
    R/db-lib.R:85:1
    R/db-lib.R:88:1
    R/db-lib.R:109:1
    ... and 134 more lines

  ✖ 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/polyhedra-lib.R:142:38

  ✖ 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/db-lib.R:435:21
    R/polyhedra-lib.R:162:17
    R/polyhedra-lib.R:300:21
    R/polyhedra-lib.R:473:26
    R/polyhedra-lib.R:477:28
    ... and 2 more lines

  ✖ not import packages as a whole, as this can cause name clashes between the imported packages. Instead, import
    only the specific functions you need.
  ✖ fix this R CMD check NOTE: installed size is 13.6Mb sub-directories of 1Mb or more: extdata 13.4Mb
──────────────────────────────────────────────

Reviewers: Due date:

kenarab commented 6 years ago

Great Noam.

We already included Badges, and submitted a new release on CRAN with minor corrections.

At this stage, as the package is a compilation of previous work, we will not write a paper so we don't need at these time a joint JOSS publication. We have the plan to implement [Conway-Hart polyhedra notation] in a not defined yet future version. At that moment we will be looking forward to publish a paper.

We will need some time for comply with the suggestions made. I will keep you posted.

Best, Ale.

2018-03-13 23:59 GMT-03:00 Noam Ross notifications@github.com:

Editor checks:

Fit: The package meets criteria for fit http://policies.md#fit and overlap http://policies.md#fit

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

Glad we're getting this going again!

You must include a paper.md http://joss.theoj.org/about#paper_structure so our reviewers can review it on behalf of JOSS if you want joint JOSS publication. This does need to happen before reviewers are assigned, but I will go ahead and look for reviewers and have them start once you have the paper in place.

I note that while you have CI set up, you don't have Travis-CI or Codecov badges in your package README. Please add these - they make it easier to see package status at a glance. Also, you may now add the ROpenSci status badge:

It appears goodpractice::gp() bug is actually in the covr package. For some reason it fails under the the first test, even though your tests pass with flying colors. I suggest you submit a bug report to covr. Otherwise, Here are goodpractice checks without using the covr. Nothing serious here - please fix the small stuff. Reviewers should consider whether changes to code style or package imports are necessary.

Regarding the large file issue: if you have a lot of large polyhedra you would like to serve to users but cannot get on CRAN, may I suggest adding an after-the-fact download function like download_polyhedra(). This is a strategy used by several others; the recently accepted rtika packages uses this approach https://github.com/ropensci/onboarding/issues/191#issuecomment-370656143 to deliver a large .jar after installation. Your decision on this can wait until after reviews are in an reviewers have weighed in.

── GP Rpolyhedra ─────────────────────────────────────

It is good practice to

✖ 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. ✖ 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/db-lib.R:32:1
R/db-lib.R:67:1
R/db-lib.R:85:1
R/db-lib.R:88:1
R/db-lib.R:109:1
... and 134 more lines
✖ 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/polyhedra-lib.R:142:38
✖ 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/db-lib.R:435:21
R/polyhedra-lib.R:162:17
R/polyhedra-lib.R:300:21
R/polyhedra-lib.R:473:26
R/polyhedra-lib.R:477:28
... and 2 more lines
✖ not import packages as a whole, as this can cause name clashes between the imported packages. Instead, import only the specific functions you need. ✖ fix this R CMD check NOTE: installed size is 13.6Mb sub-directories of 1Mb or more: extdata 13.4Mb ──────────────────────────────────────────────

Reviewers: Due date:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/157#issuecomment-372888862, or mute the thread https://github.com/notifications/unsubscribe-auth/ACzA1nrQDhjy6ra2Qq3RRPCoHoCdvECgks5teIesgaJpZM4QYpu6 .

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

noamross commented 6 years ago

@kenarab It took a little longer than I hoped but I have two great reviewers lined up. We can move ahead with starting the review (the GP outputs are minor enough that they can be corrected by the end of the review). I just want to check that you have a stable branch that they can work with.

kenarab commented 5 years ago

Hello @noamross. We are finally uploading a new version with all observations you made implemented.

Here is a link to the current release (v0.3.0)

We included a function switchToFullDatabase() for switching to fullDB, which downloads from a separate repository. By default, the packages installs with a minimal db with 120 polyhedra. There are many configurations possible for users but also for development proposes. All polyhedra is scraped in its own serialized file, so different packing strategies could be implemented in a future, including a scrape-on-demand, for example.

Other interesting feature that worth mentioning is polyhedra RGL models are now modified with transformationMatrix, so common strategies for managing openGL models can be applied.

The rest of modifications are technical, We think V0.3.0 are a good starting point for reviewers and doesn't worth for reviewers to check it out.

Thank you very much for your time and guidance. We hope the package is Ropensci compliant, and we can finish the process to publish within the project.

About JOSS, we didn't upload any paper because we don't think the contribution in this package has to be published in a peer reviewed paper at this point on time. Best,

Leonardo & Alejandro.

noamross commented 5 years ago

Thank you @kenarab. I see that your overall test coverage is still low at about 30%. Is there a reason for this? Sometimes this occurs because some of the package-building code ends up in the package itself. But coverage less than 75% or so is problematic.

kenarab commented 5 years ago

Hi Noam.

The code not covered is infrastructure for the core functionality. That’s why we didn’t included it in test cases, but of course it is a good practice to be covered.

We are coding the new test cases in the next days.

We are now in ic18 conference presenting Rpolyhedra.

Thank you, All the best, Ale.

El 3 dic. 2018, a la(s) 13:05, Noam Ross notifications@github.com escribió:

Thank you @kenarab. I see that your overall test coverage is still low at about 30%. Is there a reason for this? Sometimes this occurs because some of the package-building code ends up in the package itself. But coverage less than 75% or so is problematic.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

kenarab commented 5 years ago

Hi Noam. We just finnished a sprint for code coverage. Now is above 75%. Will be better in the future but maybe should be enough for Ropensci. Also checked goodpractices, lintr, etc.

The development branch is https://github.com/qbotics/Rpolyhedra/tree/dev-0.3.1 CRAN updates are offline now, so we will upload the package 0.3.1 to CRAN when it is available again (01/02/2019)

I hope you a great 2019.

Best, Ale.

El jue., 6 dic. 2018 a las 13:10, alejandro baranek (< alejandrobaranek@gmail.com>) escribió:

Hi Noam.

The code not covered is infrastructure for the core functionality. That’s why we didn’t included it in test cases, but of course it is a good practice to be covered.

We are coding the new test cases in the next days.

We are now in ic18 conference presenting Rpolyhedra.

Thank you, All the best, Ale.

El 3 dic. 2018, a la(s) 13:05, Noam Ross notifications@github.com escribió:

Thank you @kenarab https://github.com/kenarab. I see that your overall test coverage is still low at about 30% https://codecov.io/github/qbotics/Rpolyhedra. Is there a reason for this? Sometimes this occurs because some of the package-building code ends up in the package itself. But coverage less than 75% or so is problematic.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/157#issuecomment-443764103, or mute the thread https://github.com/notifications/unsubscribe-auth/ACzA1rql4Z-qiSxjXP3emPjcuqnrrQpJks5u1UvMgaJpZM4QYpu6 .

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | rpolyhedra https://qbotics.shinyapps.io/rpolyhedra-explorer/?_inputs_&polyhedron_color=%22%23FF0000FF%22&polyhedron_name=%22Triakis%20Tetrahedron%22&polyhedron_source=%22dmccooey%22&show_axes=false | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

noamross commented 5 years ago

Thank you @kenarab . As it's been a while since I originally contacted them I will see whether the original reviewers are still available. Also, a few notes, which I will ask the reviewers to consider. I would recommend not incorporating them until reviews are in, as well, so you can assess the feedback all together:

Function documentation is pretty sparse, in that most function descriptions are one-line. However you also include documentation for a large number of non-exported functions that are developer facing. This makes for confusing navigation of help. I'd suggest leaving this documentation only in the source code and not in man pages, which you can do with the @noRd tag. Then focus on expanding the documentation of the smaller number of exported functions. For instance, there is not documentation on the R6 methods available for the Polyhedron class.
Similarly, the vignette includes calls to non-exported functions and has little explanatory text. Removing the section on non-exported function to a developer's vignette or appendix that makes it clear these are internal, package-building functions would improve clarity, and the vignette should spend more effort on explaining the exported functions, the R6 class and how and when to use to use them.
As I mentioned above, the vignette is more useful if it is a fully compiled R Markdown with inputs and outputs. rgl can do this with WebGL. For instance, the following code will produce R markdown HTML output with manipulatable WebGL:

    ```{r setup, include=FALSE}
    library(rgl)
    library(Rpolyhedra)
    setupKnitr()

```{r tetra, webgl=TRUE}
pol <- getPolyhedron(polyhedron.name = "tetrahedron")$getRGLModel()
shade3d(pol, color = "red")
```

kenarab commented 5 years ago

Hello @noamross:

Thank you for your time and your coaching and tips! We applied all of them. You can see them in this branch. Also we just uploaded a new release (v0.4.0) to CRAN.

We hope reviewers (originals or new ones) can take the time to assess Rpolyhedra.

Happy new year for all the Ropensci team.

Best, Ale.

noamross commented 5 years ago

@yulijia and @schloerke have agreed to review. Due date: 2019-01-30.

kenarab commented 5 years ago

Thank you @noamross!

And than you @yulijia and @schloerke for accepting being our reviewers.

Best, Ale.

yulijia 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
[x] 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.
[x] Functionality: Any functional claims of the software been confirmed.
[x] Performance: Any performance claims of the software been confirmed.
[x] 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

Final approval (post-review)

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

Estimated hours spent reviewing: 2.5 hours

Review Comments

I used Fedora release 28 and I was able to install the package from Github without any problem by devtools.

Function and variable naming is proper.
README.rmd is clear and easy to follow steps for installations.
devtools::check passes without any warnings or any errors.

This is quite a nice job, all demos are run successfully and very beautiful. The code is neat, but some of the package help pages make me confused. The following comments are addressing minor things that I didn't understand, it may need to be improved.

The help page of Rpolyhedra-package and .onLoad() are same. What does .onLoad() used for?
How to use scrapePolyhedra() and scrapePolyhedraSources()? Any examples in the help page? Or maybe they are internal functions?

schloerke 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
[x] 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.
[x] Functionality: Any functional claims of the software been confirmed.
[x] Performance: Any performance claims of the software been confirmed.
[x] 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

Final approval (post-review)

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

Estimated hours spent reviewing: 3

Review Comments

README

Change to "scraped from publically available"
The install.packages line is not needed in instructions in development version. devtools::install_github will install them automatically.
Had to eventually use devtools::install_github("qbotics/Rpolyhedra", build_opts=NULL) as the vignette was not created using the default arguments. (Could also use the remotes package instead of devtools, as devtools is trying to not be responsible for install_github anymore.)

switchToFullDatabase failed to work on MacOS


> switchToFullDatabase()
Full Database needs to download data to home folder.  Agree [y/n]?:y
trying URL 'https://api.github.com/repos/qbotics/RpolyhedraDB/zipball/v0.4.0'
downloaded 5.1 MB

Error in file.copy(from = file.path(td, tmp.db.path, files.to.copy), to = getUserSpace(), : more 'from' files than 'to' files


#### Documentation

* The title for the exported function documentation to **not** be the function name (ex: `getAvailablePolyhedra()`).  Typically, this short title is <= 5 words. (ex: `Get available polyhedra`)
* Reformat the start of zzz.R to the code below.
  * By setting the "function" to `"_PACKAGE"`, it will inherit from the `DESCRIPTION` file.
  * use `\itemize` for bulleted lists.
  * `.onLoad` should not be documented

```r
#' @details
#' A polyhedra database scraped from:
#' \itemize{
#'    \item http://paulbourke.net/dataformats/phd/: PHD files as R6 objects and 'rgl'
#'          visualizing capabilities. The PHD format was created to describe the geometric
#'          polyhedra definitions derived mathematically <http://www.netlib.org/polyhedra/>
#'          by Andrew Hume and by the Kaleido program of Zvi Har'El.
#'    \item http://dmccooey.com/Polyhedra: Polyhedra text datafiles.
#' }
#'
#' @docType package
#' @importFrom R6 R6Class
#' @importFrom futile.logger flog.info
"_PACKAGE"

#' Executes code while loading the package.
#'
#' @param libname The library name
#' @param pkgname The package name
#' @noRd
.onLoad <- # (rest of code)

Examples

Not all exported functions contained an example. Missing for getPolyhedraObject and switchToFullDatabase. But both functions do not require examples IMO.
Do not library your package within the example for polyhedronToXML
Print the final output at the end of your examples. It's not much, but it lets the user know it worked.
- ex: Add this final example line #'cubes to getAvailableSources
- Do this for
- getAvailablePolyhedra
- getAvailableSources

Community Guidelines

Missing contribution guidelines in CONTRIBUTING or README.md

Automated Tests

Unfortunately, these do not pass on my machine. One failed.
- Update: Passes now. Originally failed due to personal machine setup.

> test_check("Rpolyhedra")
trying URL 'https://api.github.com/repos/qbotics/RpolyhedraDB/zipball/v0.4.0'
downloaded 5.1 MB

── 1. Error: test on package lib functions (@test_package_lib.R#18)  ───────────
more 'from' files than 'to' files
1: with_mock(`Rpolyhedra::getUserSpace` = function() {
       .tmp.home.dir
   }, testthat::expect(with_mock(`Rpolyhedra::getDataEnv` = function() {
       "HOME"
   }, with_mock(`Rpolyhedra::checkDatabaseVersion` = function() {
       "UPDATE"
   }, downloadRPolyhedraSupportingFiles() %in% c("SUCCESS", "NOT_AVAILABLE"))), failure_message = "downloadRPolyhedraSupportingFiles error")) at testthat/test_package_lib.R:18
2: eval(code[[length(code)]], parent.frame())
3: eval(code[[length(code)]], parent.frame())
4: testthat::expect(with_mock(`Rpolyhedra::getDataEnv` = function() {
       "HOME"
   }, with_mock(`Rpolyhedra::checkDatabaseVersion` = function() {
       "UPDATE"
   }, downloadRPolyhedraSupportingFiles() %in% c("SUCCESS", "NOT_AVAILABLE"))), failure_message = "downloadRPolyhedraSupportingFiles error")
5: as.expectation.logical(ok, failure_message, info = info, srcref = srcref)
6: with_mock(`Rpolyhedra::getDataEnv` = function() {
       "HOME"
   }, with_mock(`Rpolyhedra::checkDatabaseVersion` = function() {
       "UPDATE"
   }, downloadRPolyhedraSupportingFiles() %in% c("SUCCESS", "NOT_AVAILABLE")))
7: eval(code[[length(code)]], parent.frame())
8: eval(code[[length(code)]], parent.frame())
9: with_mock(`Rpolyhedra::checkDatabaseVersion` = function() {
       "UPDATE"
   }, downloadRPolyhedraSupportingFiles() %in% c("SUCCESS", "NOT_AVAILABLE"))
10: eval(code[[length(code)]], parent.frame())
11: eval(code[[length(code)]], parent.frame())
12: downloadRPolyhedraSupportingFiles() %in% c("SUCCESS", "NOT_AVAILABLE")
13: downloadRPolyhedraSupportingFiles()
14: file.copy(from = file.path(td, tmp.db.path, files.to.copy), to = getUserSpace(),
       recursive = TRUE)
15: stop("more 'from' files than 'to' files")

══ testthat results  ═══════════════════════════════════════════════════════════
OK: 651 SKIPPED: 0 FAILED: 1
1. Error: test on package lib functions (@test_package_lib.R#18)

Error: testthat unit tests failed
Execution halted

1 error ✖ | 0 warnings ✔ | 0 notes ✔

Packaging guidelines

Code style
- Mix of camel case, dot case, and snake case
- Most of the functions and variables are camel case. Might keep camel case for ease of use. (I prefer snake case.)
- I understand the use of starting with a capital letter to signify that the variable is a R6 class and not a function.
- Please do not use period separation.
- Please be consistent with your case selection.
- Use lintr's .lintr file with the contents of the code below and solve all remaining style lints found with lintr::lint_package()
```
linters: with_defaults(
snake_case_linter, # do not allow snake case
camel_case_linter = NULL, # allow camel case
multiple_dots_linter, # do not allow dot sep
commented_code_linter = NULL, # comment code is ok
NULL
)
```

Final thoughts

I've made a PR with most of the minor changes implemented.
The consistency of the code style is up to the authors / rOpenSci. It should be addressed or acknowledged that it's not going to change.

Remaining fixes needed for approval

[x] Merge PR (or make similar changes) (minor changes)
[x] Fix broken test on MacOS
[x] Fix broken function switchToFullDatabase on MacOS
[x] Add missing contribution guidelines in CONTRIBUTING or README.md. (9.1 Make your repo contribution and collaboration friendly)

Update Feb 4, 2019

My personal ~/.R folder was not writable. This should be writeable by default. This was my problem. By fixing the folder permissions, the test suite and switchToFullDatabase function works as expected.

kenarab commented 5 years ago

Thank you for your feedback and advices @yulijia and @schloerke! I just been father of tweens on last wednesday (1-30-2019) so I been full offline for some days. In next days we will analyse your feedback and write back.

We merged the PR @schloerke made, but wondering about which style code did you use. We are based on google style code (GSC). I don't know if RopenSci have a style code down to the level of GSC. Capital Letter starting convention was defined for R6 classes, as there was no reference to R6 classes in GSC.

It may be possible we have some unconsistency in the code, we will check. Maybe will change lintr config @schloerke PR us.

About broken tests and switchToFullDatabase is strange it is not working: we are using Travis CI and working on MacOS for developing and for every release we run all kinds of checks (A call to switchToFullDatabase is included in testcases) for it to work before publishing. And then, for publishing the package on CRAN all test should work ok.

In which branch did you were working whe you had tests problems?

Thank you! We will reach you on next days.

kenarab commented 5 years ago

about @yulijia question 2:

How to use scrapePolyhedra() and scrapePolyhedraSources()? Any examples in the help page? Or maybe they are internal functions?

They are development functions. We applied @noRd as @noamross suggested so there are no documentation. For a package like this, intended to be reproducible, we would prefer have a call like @devRd (A vigentte for developers) for any dev or collaborator who wants to reproduce the backstage would be able to do it. Reproducible features for the scraping process is not finished yet. We will research on this while working on this review.

noamross commented 5 years ago

@kenarab First, congratulations on becoming a new father, twice! 👶👶 Thank you @yulijia and @schloerke for your reviews.

Regarding code style: consistency is generally our rule. The pattern we find most frequent is people use one form for user-facing functions, a different form for internal functions and R6 or S4 types. With the split in this package user-facing and developer-facing functions, I mostly think that is important that there is consistency within each of these.

Per the reviewer's questions above, the CONTRIBUTING.md file would be an excellent place to explain the overall scraping/developer workflow, referring the reader to the documentation in the source code for details on specific functions.

@sckott is our expert on mocking test functions, he might have some insight into the test failure above.

schloerke commented 5 years ago

Updating here as well. My personal configuration had ~/.R require sudo to make new folders. This is non standard. By fixing my folder permissions, it fixed the function issue and test suite failure. Sorry for the odd scare!

kenarab commented 5 years ago

Thank you @noamross! The twins are in neonatology, but they are very good. They are growing really fast and will go to home soon.

Dear @yulijia and @schloerke and @noamross . We implemented most of suggestions.

We configured lintr for CamelCase and DotCase. Removed Snake case and found some internal inconsistencies, but most were related to dependencies, specifically testthat. For avoiding lintr messages added testthat spacename in that callings. But it is specific to some lintr version and others no.
About code style we still using google code style. With exception of R6 objects, as discussed previously. This was included in CONTRIBUTING.md as matters to developers only
We included a CONTRIBUTING.md which explain some basics of the package for developers. We will improve the description in next versions. For example, for people wanting to program scrapers for other sources.
About @yulijia question The help page of Rpolyhedra-package and .onLoad() are same. What does .onLoad() used for?: @schloerke the other reviewer made a PR fixing this documentation issue. .onLoad function is a standard function for R package with initialization scripts regarding a package.

Thank you very much for your time.

Best, Ale

PS. We made a dev-ropensci branch for developing suggestions. We are merging in master when they are ready. You can check master branch, but for I suggest PR should be made over dev-ropensci branch instead of master

yulijia commented 5 years ago

Hi @kenarab, sorry for the delayed reply. Thank you for answer all my questions, I have checked all the document again, and they are fine now. Congratulation for the twin.

noamross commented 5 years ago

Thank you all. @yulijia and @schloerke, please let us know if @kenarab's changes have addressed all your comments. If you have any additional comments on interface or use cases that you may not have gotten to while working through testing and documentation issues, feel free to provide these, as well.

schloerke commented 5 years ago

Good to me!

Took me a couple times to realize the Google Coding Style is a mixed style to help identify a variable vs a package function. I've been trained on a "all or nothing" style (preferring snake case, which google does not like).

kenarab commented 5 years ago

Dear all:

We have made cleanup of Rpolyhedra repo. If you have uncommited code or PR, please clone the repo again before making changes.

Best, Ale.

On Thu, Feb 14, 2019 at 12:35 PM Barret Schloerke notifications@github.com wrote:

Good to me!

Took me a couple times to realize the Google Coding Style is a mixed style to help identify a variable vs a package function. I've been trained on a "all or nothing" style (preferring snake case, which google does not like).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/157#issuecomment-463672267, or mute the thread https://github.com/notifications/unsubscribe-auth/ACzA1qWqt3FYATClpIzIkAlYxqQb_Sv0ks5vNYI_gaJpZM4QYpu6 .

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | rpolyhedra https://qbotics.shinyapps.io/rpolyhedra-explorer/?_inputs_&polyhedron_color=%22%23FF0000FF%22&polyhedron_name=%22Triakis%20Tetrahedron%22&polyhedron_source=%22dmccooey%22&show_axes=false | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

noamross commented 5 years ago

Approved! 🎉Thanks @kenarab for submitting and going through what has been a long read, and and @yulijia and @schloerke 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'll be made admin once you do.
[ ] Add the rOpenSci footer to the bottom of your README " [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)"
[ ] Fix any links in badges for CI and coverage to point to the ropensci URL.
[ ] We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#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.

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). More info on this here.

Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. (https://ropensci.org/blog/). If you are interested, @stefaniebutland will be in touch about content and timing.

We recently published a gitbook with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

kenarab commented 5 years ago

Wow! Gr8!! Thank you again @noamross, @schloerke @yulijia for your time.

To-dos:

[X] 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'll be made admin once you do.

[X] Add the rOpenSci footer to the bottom of your README "[![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)"

[X] Fix any links in badges for CI and coverage to point to the ropensci URL.

[X] We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#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.

[X] 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). More info on this here.

I added the 3 of you as reviewers. If you want changes, please make a PR with them.

Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. (https://ropensci.org/blog/). If you are interested, @stefaniebutland will be in touch about content and timing.

We recently published a gitbook with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

Excellent! We will read the corresponding chapters and planning to write a blog post for being included in (https://ropensci.org/tech-notes/) and/or (https://ropensci.org/tech-notes/)(https://ropensci.org/blog/)

As I already transferred the repo I need admin privileges for inviting @leobelen to rOpenSci and giving him admin access too for pushing the changes requesting in onboarding process.

Best, Ale.

noamross commented 5 years ago

Great. Admin privileges assigned. Please don't include editors as rev contributors, we would be on too many 🙂 .

kenarab commented 5 years ago

Thank you, Noam!

I looked through ?personand saw there is no role for editor. So you will not be mentioned in authors@R, ok?

Can you please invite @leobelen to RopenSci and add him to Rpolyhedra team? I see have no privileges for doing and he is actually an author of the package.

Best, Ale.

On Mon, Feb 25, 2019 at 4:39 PM Noam Ross notifications@github.com wrote:

Great. Admin privileges assigned. Please don't include editors as rev contributors, we would be on too many 🙂 .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/157#issuecomment-467152685, or mute the thread https://github.com/notifications/unsubscribe-auth/ACzA1uGnmCB4LkGk_If753CU-m1A7dayks5vRDvXgaJpZM4QYpu6 .

-- alejandro baranek @ken4rab https://twitter.com/ken4rab qbotics http://qbotics.tumblr.com/ | rpolyhedra https://qbotics.shinyapps.io/rpolyhedra-explorer/?_inputs_&polyhedron_color=%22%23FF0000FF%22&polyhedron_name=%22Triakis%20Tetrahedron%22&polyhedron_source=%22dmccooey%22&show_axes=false | surferinvaders http://surferinvaders.tumblr.com | algebraic-soundscapes http://imaginary.org/content/algebraic-soundscapes | surfer-shuffle http://imaginary.org/program/surfer-shuffle

stefaniebutland commented 5 years ago

@kenarab I'm rOpenSci's Community Manager, here to say we would love to feature a post about Rpolyhedra on our blog.

This link will give you many examples of blog posts by authors of peer-reviewed packages so you can get an idea of the style and length you prefer: https://ropensci.org/tags/software-peer-review/. Here are some technical and editorial guidelines for contributing a blog post: https://github.com/ropensci/roweb2#contributing-a-blog-post.

Please let me know when you might want to submit a draft. Happy to answer any questions.

ropensci / software-review

Rpolyhedra #157

Summary

Requirements

Publication options

Detail

Editor checks:

Editor comments

Editor checks:

Editor comments

Package Review

Documentation

Functionality

Final approval (post-review)

Review Comments

Package Review

Documentation

Functionality

Final approval (post-review)

Review Comments

README

Examples

Community Guidelines

Automated Tests

Packaging guidelines

Final thoughts

Update Feb 4, 2019