Conversation
- drop VignetteBuilder: knitr b/c no vignettes to build - added future and future.apply to imports - replace remote install of rads.data with rads b/c rads is needed too and auto installs rads data - removed self reference to github::PHSKC-APDE/apde.chi.tools - changed role = 'auth' to role = 'aut' - tweaked the Description paragraph, which must be in complete sentences
- specify namespace for substrRight, calc,chi_cols, compare_estimate, get_population, - added @importFrom for crossing & substrRight - replace .() with list() - added trend.span parameter that was missing - name space updated by devtools::document() due to importFrom
- to avoid warnings about missing visible bindings for global variables
- chi_generate_trend_years does not exist for testing
- added year.span as explicit argument to chi_generate_tro_shell() and test - replaced `span` (undefined var) with `trend.span) in chi_process_trends
- Replaced `assign()` calls with local variable assignments within `chi_calc` function to improve encapsulation and avoid unintended side effects in the global environment - Simplified code for better readability and maintainability - Removed unnecessary cleanup of global variables (`rm()`) - Eliminating globals within package functions is important for ensuring predictability, avoiding conflicts, and maintaining clean, self-contained functions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was going to work on addressing a few issues on this package and found that it failed
devtools::check()with multiple errors, warnings, and notes.I methodically addressed the identified issues and saved them as discrete commits.
The only commit that I believe may have impacted functionality is Fixes for chi_generate_tro_shell test failure, because there were inconsistencies across multiple files.
After making these changes,
devtools::check()returns 0 errors, 0 warnings, and 0 notes.Documentation is needed for some functions (e.g.,
chi_calc()) and more extensive tests are needed, but it is now it is in good enough shape that some issues can be addressed with the ability to test the package afterward.Finally, I added a
devbranch, that is simply a copy ofmainat this point. I think the best practice for a package that is used by others is to always have a main branch that works and a dev branch where bug fixes, improvements, new functions, etc. can be added and thoroughly tested. Hence the reason why this pull request is intodev.