87 rewrite pkg structure #92
Merged
Conversation
This file contains 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
Aeilert
requested changes
Apr 17, 2024
There was a problem hiding this comment.
Thanks @ohjakobsen!
This looks good. Just a few minor comments:
- Please use
@export,@keywords internalor@noRd-tags for all functions. I appears that particularly@noRdare missing in thefct_*-files. - I think we should be consistent in the use of
::vs.@importand@importFrom. I prefer::since it makes the imports of a package explicit. - Please add an issue for the unit tests you think we should create.
Aeilert
approved these changes
Apr 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR moved all functions that earlier were put in
./inst/appand./inst/app/Rinto./R. This way, all internal functions should be available within the appsui()andserver()functions. To align with best practices for R packages and remove notes and warnings from R CMD check, several changes has been made:global.Rfile has been removed and it's functions has been added elsewhere in the code, including in theserver()andrun_pioneer()functionsconditionalUI.Rhas been added toserver(). The code for theserver()function can be split into Shiny modules in a later PRrequire()has been replaced with@importor@importFromstatements or the use of::. The package now includes far more packages in the Imports section to make sure that all packages needed to run the app are availableThis PR does not include new tests. New test should be written for most functions. However, the purpose of this PR is to move all functions out of
./inst/app. We should have a review of how functions can be optimised now that they are all internal functions. This should be a separate PR where we also introduce new tests.