Skip to content

Conversation

@nikolas-burkoff
Copy link
Contributor

@nikolas-burkoff nikolas-burkoff commented Jul 6, 2022

Closes #81

image

This function is only used in .onLoad

FYI @arkadiuszbeer

R/utils.R Outdated
Comment on lines 79 to 85
inst_file <- file.path(system.file(package = pkg), file_name)

if (file.exists(base_file)) {
return(base_file)
} else if (file.exists(inst_file)) {
} else if (file.exists()) {
return(inst_file)
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gogonzo do we actually need inst_file? Or should I remove lines 79 + 83 + 84 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, remove them as in the compiled package there is no inst directory.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

Code Coverage Summary

Filename                                 Stmts    Miss  Cover    Missing
-------------------------------------  -------  ------  -------  -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/as_cdisc.R                                39       4  89.74%   107-110
R/Callable.R                                45       0  100.00%
R/CallableCode.R                            36       2  94.44%   26, 63
R/CallableFunction.R                        88       3  96.59%   160-162
R/CallablePythonCode.R                      58      58  0.00%    21-223
R/CDISCTealData.R                           79      12  84.81%   41, 47-50, 55, 128, 131, 134, 266-268
R/CDISCTealDataConnector.R                  20       3  85.00%   31, 36, 49
R/CDISCTealDataset.R                        46      11  76.09%   108-115, 208-210
R/CDISCTealDatasetConnector.R               26       1  96.15%   116
R/CodeClass.R                              111       1  99.10%   157
R/data_label.R                              32       9  71.88%   35-39, 58-61, 101
R/deep_clone_r6.R                            9       0  100.00%
R/get_attrs.R                                2       2  0.00%    12-47
R/get_code.R                               173      21  87.86%   87, 140-143, 196-197, 201, 207-208, 212, 266, 297, 333, 337, 372, 381-385
R/get_dataname.R                             4       0  100.00%
R/get_dataset_label.R                        3       0  100.00%
R/get_dataset.R                             13       8  38.46%   37, 54, 80-86
R/get_datasets.R                            10       5  50.00%   85, 107-111
R/get_key_duplicates.R                      37       7  81.08%   42-48, 55-56
R/get_keys.R                                15       7  53.33%   72-73, 130-150
R/get_raw_data.R                            24      11  54.17%   109-122
R/include_css_js.R                           9       1  88.89%   20
R/is_pulled.R                                4       0  100.00%
R/JoinKeys.R                               128       4  96.88%   219, 282-321
R/load_dataset.R                            25      18  28.00%   60-65, 89-222
R/MAETealDataset.R                         137      57  58.39%   57, 118, 156-211, 227-232, 239-248, 285, 326-342
R/mutate_dataset.R                          18       0  100.00%
R/set_args.R                                10       5  50.00%   42-46
R/teal_data.R                               21       3  85.71%   101-103
R/TealData.R                               271     116  57.20%   87, 99, 236, 248-316, 330, 333, 389-396, 426-431, 433, 435-440, 442, 459-504
R/TealDataAbstract.R                       231      13  94.37%   86-89, 214-217, 428, 453-457, 479, 485
R/TealDataConnection.R                     296     179  39.53%   58-59, 64, 67, 70, 106-162, 182, 185-187, 193-199, 204-206, 232, 237, 253-276, 286, 299, 320, 324-329, 332-335, 357-359, 363-370, 373-376, 391-405, 424-425, 445-516, 534-542, 544, 548-563, 566-569, 601, 607-611, 625, 660-662, 671-673
R/TealDataConnector.R                      195     101  48.21%   167, 179, 183, 196, 199-208, 210, 218-227, 310-314, 372-476
R/TealDataset.R                            376      23  93.88%   148-158, 432-436, 492-501, 551
R/TealDatasetConnector_constructors.R      300      53  82.33%   176-212, 262, 830-835, 1033-1109
R/TealDatasetConnector.R                   341      90  73.61%   122, 179, 248, 262, 267, 281, 473, 496-534, 565, 580-610, 700, 710, 719-726, 739, 754-781
R/to_relational_data.R                      54       7  87.04%   35-36, 40, 99, 106, 112, 127
R/topological_sort.R                        32       4  87.50%   53-56
R/utils.R                                   49       9  81.63%   22-23, 27, 76-83
R/validate_data_args.R                      34       0  100.00%
R/zzz.R                                      6       6  0.00%    4-12
TOTAL                                     3407     854  74.93%

Results for commit: e9a974b

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

Unit Tests Summary

       1 files       27 suites   51s ⏱️
   319 tests    319 ✔️ 0 💤 0
1 098 runs  1 098 ✔️ 0 💤 0

Results for commit 045b3eb.

♻️ This comment has been updated with latest results.

@nikolas-burkoff nikolas-burkoff requested a review from gogonzo July 6, 2022 09:14
@Polkas
Copy link
Contributor

Polkas commented Jul 6, 2022

The base::system.file is overwritten (e.g. when devtools::load_all is used) by the pkgdown version:

> system.file
function (..., package = "base", lib.loc = NULL, mustWork = FALSE) 
{
    if (!(package %in% dev_packages())) {
        return(base::system.file(..., package = package, lib.loc = lib.loc, 
            mustWork = mustWork))
    }
    if (dots_n(...) && is_string(..1)) {
        if (is_string("inst", ..1) || grepl("^inst/", ..1)) {
            cli::cli_abort(c("Paths can't start with `inst`", 
                i = "Files in `inst` are installed at top-level."))
        }
    }
    pkg_path <- find.package(package)
    files_inst <- file.path(pkg_path, "inst", ...)
    present_inst <- file.exists(files_inst)
    files_top <- file.path(pkg_path, ...)
    present_top <- file.exists(files_top)
    files <- files_top
    files[present_inst] <- files_inst[present_inst]
    files <- files[present_inst | present_top]
    if (length(files) > 0) {
        normalizePath(files, winslash = "/")
    }
    else {
        if (mustWork) {
            cli::cli_abort("Can't find package file.", call = NULL)
        }
        ""
    }
}
<bytecode: 0x1374fa068>
<environment: namespace:pkgload>

There are at least two solutions:

  • remove the lines suggested by Nick
  • add base:: prefix to system.file calls
  • remove the function :)

R/utils.R Outdated
checkmate::assert_string(file_name)
base_file <- system.file(file_name, package = pkg)
inst_file <- system.file("inst", file_name, package = pkg)
inst_file <- file.path(system.file(package = pkg), file_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
inst_file <- file.path(system.file(package = pkg), file_name)

get_package_file <- function(pkg = NULL, file_name = NULL) {
checkmate::assert_string(pkg)
checkmate::assert_string(file_name)
base_file <- system.file(file_name, package = pkg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
base_file <- system.file(file_name, package = pkg)
base_file <- base::system.file(file_name, package = pkg)

@nikolas-burkoff nikolas-burkoff enabled auto-merge (squash) July 6, 2022 11:41
@nikolas-burkoff nikolas-burkoff merged commit 934974a into main Jul 6, 2022
@nikolas-burkoff nikolas-burkoff deleted the 81_inst@main branch July 6, 2022 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Files in inst are installed at top-level.

3 participants