Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new review.Rmd #117

Merged
merged 17 commits into from Sep 8, 2020
Merged

Add new review.Rmd #117

merged 17 commits into from Sep 8, 2020

Conversation

maurolepore
Copy link
Member

Thanks for this amazing work. The manuscript is super clear, and the ideas you implemented since last time I saw this project look great. Here I'll add a few comments as I review the code. I start this PR as a draft. I'll let you know when I'm ready to start the discussion. For now I'll collect my thoughts in the file review.md.

@maurolepore maurolepore marked this pull request as ready for review September 5, 2020 21:56
@maurolepore
Copy link
Member Author

Here I share my review. You may want to see two places -- my comments (review.md) and my changes (Files changes).

Thanks a lot for including me in this excellent work! 😄

@@ -11,3 +11,7 @@
^README\.Rmd$
^\.github$

^review\.md$
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading my comments, you may remove the file review.md and also its entry in .Rbuildignore.

- r: 3.2
- r: 3.1

- r: 3.5
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you fix the issue by which we need R >= 3.5.0, then you can list here the earlier versions we support, e.g.:

  - r: 3.4
  - r: 3.3

DESCRIPTION Show resolved Hide resolved
DESCRIPTION Show resolved Hide resolved
DESCRIPTION Show resolved Hide resolved
DESCRIPTION Show resolved Hide resolved
Comment on lines +108 to +115
#' Title
#'
#' Description
#'
#' @source ?
#'
#' @family database datasets
"genus_family"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do something like this with all undocumented datasets.

Comment on lines +1 to +2
#' @importFrom utils data
NULL
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do something like this for each function from an external package that you want to use here; but you can avoid this using the syntax packagename::functionname().

@@ -1,47 +0,0 @@
url: https://forestgeo.github.io/allodb/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seemed to be unused, outdated, and corrupted (formatting issues). The easiest solution for now was to remove it. An empty _pkgdown.yml still works -- it just produces a default site. Look at it by opening data/index.html on your web browser.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @maurolepore on all this. As you may have noticed we merged your pull request and tried to fix some functions for a general improve. I still have few questions about publishing the website:

  1. Which Branch/folder should I select? If I choose master/root then the website looks as I built it using the corrupt _pkgdown.yaml file (live now). if I choose master/docs it looks like the default built as shown on docs/index.html. I read the "Learn more" on Github pages, it seems it is not recommended to built a website from docs, correct?.

image

  1. I would like the final website to look like docs/index/html but change "References" to Functions" and add "Datasets" to include descriptions of tables (currently in References). I understand I need to add more tabs in the navigation bar with navbar. For that, do I write in the new _pkgdown.yaml file in root?

Another big learning curb!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I would choose /docs. As you say, that will expose the default website at the URL https://forestgeo.github.io/allodb/. In my experience, having the site in /docs is fine -- as long as you remember to update it with pkgdown::build_site() (see https://pkgdown.r-lib.org/reference/build_site.html).

I prefer to rebuild the site automatically with every push; and do that with this GitHub Action: https://github.com/r-lib/actions/tree/master/examples#build-pkgdown-site. Eventually you might want to do it this way but the result doesn't change -- it just leaves you with one thing less to worry about.

Exactly, to tweak the website you need to tweak the file _pkgdown.yaml. You may see the previous version as an example, then add a piece and rebuild with pkgdown::build_site(), repeating until you are happy. You can see examples of this file in other packages I wrote: e.g. https://github.com/forestgeo/fgeo/blob/master/_pkgdown.yml.

@@ -0,0 +1,422 @@
I believe you would like ideas about how to optimize some functions. Please let me konw which functions you mean and share with me a reproducible example (with the package <https://reprex.tidyverse.org/>) showing me how to use the function I need to inspect. Here I write what I see as I review the structure of the package. For a more complete set of recomendations see <https://devguide.ropensci.org/guide-for-authors.html> and <http://r-pkgs.had.co.nz/>.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are my comments. They explain in more detail what I comment on the tab "Files changed" of the pull request.

@cpiponiot cpiponiot merged commit 5dbc1f2 into ropensci:master Sep 8, 2020
@maurolepore maurolepore deleted the review branch December 9, 2020 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants