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

fix #115 - filter duplicate records for indicator_native_range_year #116

Merged
merged 9 commits into from
Apr 16, 2024

Conversation

mvarewyck
Copy link
Collaborator

No description provided.

@mvarewyck mvarewyck requested a review from damianooldoni April 15, 2024 12:56
@mvarewyck mvarewyck marked this pull request as draft April 15, 2024 13:01
@mvarewyck
Copy link
Collaborator Author

@damianooldoni Is there anything I can do to fix the checks?

@damianooldoni
Copy link
Collaborator

Thanks @mvarewyck. Unfortunately not. The failures are due to rgeos and rgdal which are used by sp.

Extract from the log of one of the failing actions:

* deps::.:
    * Can't install dependency rgdal
    * Can't install dependency rgeos
  * rgdal: Can't find package called rgdal, rgeos.
  * rgeos: Can't find package called rgdal, rgeos.

We know this anomaly and @soriadelva is working on this in #111

I will test your changes locally and then I will add my review.

Copy link
Collaborator

@damianooldoni damianooldoni left a comment

Choose a reason for hiding this comment

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

Thanks @mvarewyck. nice work.

I have just pushed little changes.

Notice I had to add all_of() in a dplyr::rename() to avoid a warning while running the tests of the function. Reason:

Warning (test-indicator_native_range_year.R:82:3): Test output type, class, slots and columns
Using an external vector in selections was deprecated in tidyselect 1.1.0.Please use `all_of()` or `any_of()` instead.
  # Was:
  data %>% select(taxon_key_col)

  # Now:
  data %>% select(all_of(taxon_key_col))

See <https://tidyselect.r-lib.org/reference/faq-external-vector.html>.
Backtrace:1. ├─trias::indicator_native_range_year(...) at test-indicator_native_range_year.R:82:3
  2. │ └─... %>% dplyr::rename_at(dplyr::vars(taxon_key_col), ~"key") at trias/R/indicator_native_range_year.R:108:3
  3. └─dplyr::rename_at(., dplyr::vars(taxon_key_col), ~"key")
  4.   └─dplyr:::tbl_at_vars(.tbl, .vars, .include_group_vars = TRUE)
  5.     └─tidyselect::vars_select(tibble_vars, !!!vars)
  6.       └─tidyselect:::eval_select_impl(...)
  7.         ├─tidyselect:::with_subscript_errors(...)
  8.         │ └─base::withCallingHandlers(...)
  9.         └─tidyselect:::vars_select_eval(...)
 10.           └─tidyselect:::walk_data_tree(expr, data_mask, context_mask)
 11.             └─tidyselect:::eval_c(expr, data_mask, context_mask)
 12.               └─tidyselect:::reduce_sels(node, data_mask, context_mask, init = init)
 13.                 └─tidyselect:::walk_data_tree(new, data_mask, context_mask)
 14.                   └─tidyselect:::eval_sym(expr, data_mask, context_mask)

Also wondering how to test the changes you applied to fix #115.

@mvarewyck
Copy link
Collaborator Author

Also wondering how to test the changes you applied to fix #115.

Good point, I have now included a test specific for this new feature.

@damianooldoni damianooldoni marked this pull request as ready for review April 16, 2024 09:10
@damianooldoni
Copy link
Collaborator

damianooldoni commented Apr 16, 2024

Thanks @mvarewyck to have added a test for it! I have just split a long line in a last commit (72dc97c).
I marked the draft as "ready for review". I added my positive review yesterday, so I don't need to do it again. I will merge if you also agree. The checks will still fail, but we are solving that in the meantime as well.

@damianooldoni damianooldoni self-assigned this Apr 16, 2024
@mvarewyck
Copy link
Collaborator Author

@damianooldoni Sure, good to go for me. Thanks for the quick follow up.

@damianooldoni
Copy link
Collaborator

I was merging while I realized that we didn't bump the version number 😄
Done. Now I will merge.

@damianooldoni damianooldoni merged commit c81ee6f into main Apr 16, 2024
0 of 6 checks passed
@damianooldoni damianooldoni deleted the fix_115 branch April 16, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants