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

Updated load_data.R #1592 #2771

Merged
merged 14 commits into from Mar 1, 2021
Merged

Updated load_data.R #1592 #2771

merged 14 commits into from Mar 1, 2021

Conversation

moki1202
Copy link
Collaborator

removed library functions by appropriately providing namespaces to functions.

removed library functions by appropriately providing namespaces to functions.
@ashiklom
Copy link
Member

/document

@ashiklom
Copy link
Member

Thanks @moki1202 ! This looks great!

FYI, the checks were failing because your changes call for an update to the automated package documentation (specifically, the modules/benchmark/NAMESPACE file needs to be updated to reflect the new @importFrom magrittr %>%). Typically, this means you have to run devtools::document()` from inside the package and then committing the resulting changes.

However, no need to do that here because we have a GitHub Actions bot that will do it for you! My cryptic comment above activated our documentation bot, which will automatically run devtools::document() for all the PEcAn packages and commit any changes.

replaced require(bd) with requireNamespace(bd, quietly = TRUE ) in line 43
added 'bd' in the suggests section of modules/benchmark/DESCRIPTION
added comma after testthat (>= 2.0.0) (line 37) and 'BrownDog' packageName to suggests section.
@ashiklom
Copy link
Member

/document

@infotroph
Copy link
Member

@moki1202 Can you please add magrittr, to the Imports section of the DESCRIPTION file, between current lines 23 and 24? I'd submit it as a suggested change but it's too far from any changed lines.

Extra credit if you want to bother reordering the existing import declarations so the whole listing is actually alphabetical, but clearly the rest of us haven't done that... 🙈

Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

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

Hopefully last tweaks. Thanks for sticking with this through so many rounds of 'one more thing', @moki1202 -- clearly this code was long overdue for a cleanup!

@@ -44,13 +39,13 @@ load_data <- function(data.path, format, start_year = NA, end_year = NA, site =
fcn <- match.fun(fcn1)
} else if (exists(fcn2)) {
fcn <- match.fun(fcn2)
} else if (!exists(fcn1) & !exists(fcn2) & require(bd)) {
} else if (!exists(fcn1) & !exists(fcn2) & requireNamespace(bd, quietly = TRUE)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} else if (!exists(fcn1) & !exists(fcn2) & requireNamespace(bd, quietly = TRUE)) {
} else if (!exists(fcn1) & !exists(fcn2) & requireNamespace("BrownDog", quietly = TRUE)) {

SimilarityMeasures,
zoo,
stringr
stringr,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stringr,
stringr,
tidyselect,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@infotroph ill get on this ASAP. also, the pleasure was mine, got to learn so much from you guys. thanks for being patient with me hehe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also please suggest any error fix that I can start working on after this.

@ashiklom ashiklom merged commit 8f76c8b into PecanProject:develop Mar 1, 2021
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

4 participants