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

improving .assert_package() #171

Merged
merged 14 commits into from
Jul 20, 2022
Merged

improving .assert_package() #171

merged 14 commits into from
Jul 20, 2022

Conversation

larmarange
Copy link
Owner

@larmarange larmarange commented Jul 19, 2022

new function .get_package_dependencies()

.assert_package() now checks the type of comparison for minimum version

closes ddsjoberg/gtsummary#1296

new function `.get_package_dependencies()`

`.assert_package()` now checks the type of comparison for minimum version

cf. ddsjoberg/gtsummary#1296
@larmarange
Copy link
Owner Author

New function .get_package_dependencies() returning a tibble with all depencies, minimum version and comparison operator.

.get_min_version_required() now returns the comparison operator as an attribute

The comparison operator is taken into account by .asset_package()

@ddsjoberg
Copy link
Collaborator

Wow, so so fast! Thanks for adding this! I can review tonight!

@ddsjoberg
Copy link
Collaborator

Can we re-add that unit test we excluded on GH Actions? Does it pass now?

@larmarange
Copy link
Owner Author

Can we re-add that unit test we excluded on GH Actions? Does it pass now?

I have updated the unit test. Let see if it is OK now

@ddsjoberg
Copy link
Collaborator

@larmarange everything looks perfect!

The only think I can think of is when multiples of the same package are installed (like below)

d> devtools::load_all(".")
ℹ Loading broom.helpers
d> .libPaths()
[1] "C:/Users/SjobergD/R-dev"            "C:/Program Files/R/R-4.2.1/library"
d> installed.packages() %>%
+   dplyr::as_tibble() %>%
+   dplyr::filter(Package == "broom.helpers") %>%
+   dplyr::select(Package, LibPath, Version)
# A tibble: 2 × 3
  Package       LibPath                            Version   
  <chr>         <chr>                              <chr>     
1 broom.helpers C:/Users/SjobergD/R-dev            1.7.0.9003
2 broom.helpers C:/Program Files/R/R-4.2.1/library 1.8.0     
d> .get_package_dependencies("gtsummary") %>%
+   dplyr::filter(pkg == "broom.helpers")
# A tibble: 2 × 6
  pkg_search pkg_search_version dependency_type pkg           version compare
  <chr>      <chr>              <chr>           <chr>         <chr>   <chr>  
1 gtsummary  1.6.1              Imports         broom.helpers 1.7.0   >=     
2 gtsummary  1.6.1              Imports         broom.helpers 1.7.0   >=     

There is a line that returns the most recent version (arranging by version number). But I think that the line that is picked should be the line associated with the first location listed in .libPaths(), since that is the version of the package that will be used.

For example, in the script above, I have two versions of broom.helpers installed in two locations. We should return the dependency information associated with the package saved in "C:/Users/SjobergD/R-dev" because that is the repo location that is listed first and hence the version of broom.helpers that will be used.

@larmarange
Copy link
Owner Author

There is a line that returns the most recent version (arranging by version number). But I think that the line that is picked should be the line associated with the first location listed in .libPaths(), since that is the version of the package that will be used.

installed.packages() return a list ordered according to .libPaths(). If remove_duplicates = T, now this is the first one which is returned.

R/assert_package.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@ddsjoberg ddsjoberg left a comment

Choose a reason for hiding this comment

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

Looks great!! Thank you for the great update!

@larmarange larmarange merged commit 6f09298 into main Jul 20, 2022
@larmarange larmarange deleted the assert_dep branch July 20, 2022 14:14
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.

assert_package() didn't return min version
2 participants