-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add kba #349
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
+ Coverage 86.21% 86.70% +0.49%
==========================================
Files 60 62 +2
Lines 2691 2723 +32
==========================================
+ Hits 2320 2361 +41
+ Misses 371 362 -9 ☔ View full report in Codecov by Sentry. |
package = "mapme.biodiversity") | ||
x <- read_sf(sample_path) | ||
x_area <- st_area(x) |> | ||
units::set_units("ha") |> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use units
here as this would add another dependency.
#' @export | ||
get_key_biodiversity_areas <- function(path = NULL) { | ||
|
||
if(is.null(path) || !file.exists(path)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include a line in the test script that triggers this line?
R/calc_key_biodiversity_areas.R
Outdated
verbose = mapme_options()[["verbose"]]) { | ||
|
||
key_biodiversity_areas <- key_biodiversity_areas[[1]] | ||
if (is.null(key_biodiversity_areas) | nrow(key_biodiversity_areas) == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you are at it, maybe also test this line?
@goergen95 thanks for the comments, will adapt! I also just noticed that I'm using native pipes in the unit test. I'll replace them so we're compatible with R 3.5.0 |
Yes, please do. Also, please try to pipes sparingly. We still have #98 open. |
There are also failures related to |
@goergen95 do you know how I can "force merge" this PR despite the failing check? The merge button is greyed out for me. |
Guess it requires admin privileges. Shall I (I would squash the commits to a single one)? |
Yes, please! |
No description provided.