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

Issues found while refactoring workflow.pacta.report #78

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

AlexAxthelm
Copy link
Collaborator

@AlexAxthelm AlexAxthelm commented May 30, 2024

  • import reframe from dplyr
  • Add namespace to function

Copy link

codecov bot commented May 30, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 0.81%. Comparing base (9f32a19) to head (8c9186e).

Files Patch % Lines
R/create_interactive_report.R 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main     #78   +/-   ##
=====================================
  Coverage   0.81%   0.81%           
=====================================
  Files         25      25           
  Lines       1596    1596           
=====================================
  Hits          13      13           
  Misses      1583    1583           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AlexAxthelm AlexAxthelm changed the title Issues founr while refactoring workflow.pacta.report Issues found while refactoring workflow.pacta.report May 30, 2024
@AlexAxthelm AlexAxthelm marked this pull request as ready for review May 31, 2024 18:06
@AlexAxthelm AlexAxthelm requested a review from cjyetman June 3, 2024 10:18
@AlexAxthelm AlexAxthelm merged commit b3d8006 into main Jun 3, 2024
9 of 10 checks passed
@AlexAxthelm AlexAxthelm deleted the issues-found-in-workflow.pacta.report branch June 3, 2024 12:35
AlexAxthelm added a commit to RMI-PACTA/workflow.pacta.report that referenced this pull request Jun 3, 2024
Comment on lines +1 to +7
#' @keywords internal
"_PACKAGE"

## usethis namespace: start
#' @importFrom dplyr reframe
## usethis namespace: end
NULL
Copy link
Member

Choose a reason for hiding this comment

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

sorry retrospectively, but... seems off that this file is necessary for just dplyr::reframe, probably suggests that this is unnecessarily a new and unique method (to this repo) of importing a function

Copy link
Member

Choose a reason for hiding this comment

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

My preferred method is usually to use an imports.R file: https://github.com/RMI-PACTA/r2dii.analysis/blob/main/R/imports.R

but that doesn't seem to be the Beta in pacta.portfolio.report? Unless there is a different approach already implemented here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like we were already using the R package standard of putting importFroms in NAMESPACE, but usethis::use_import_from() requires having the package-level docs.

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.

3 participants