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

Use filter functions/remove args from functions #331

Merged
merged 97 commits into from
Sep 19, 2024

Conversation

damianooldoni
Copy link
Member

@damianooldoni damianooldoni commented Jul 25, 2024

This PR solves few issues at the same time as it was difficult to disentangle them.

Forgotten to remove datapkg in tests. This solve that problem definitely.
See #330. If needed for other dplyr functions, we will add it back later.
Filtering will be discussed in a specific vignette, see #320.
Removing filtering is also needed as the function will not support anymore these args as discussed in #231.
Obsolete: filter predicates are not present anymore. Filtering deployments will be discussed in another vignette, see #320
…no observations

Notice that assignment function must still be implemented (it will be done in another branch/PR), see #328.
Filtering by `filter_observations()` if needed.
@damianooldoni
Copy link
Member Author

Thanks @sannegovaert!
Yes, many tests still fail, but it's normal. We decided to do the update to v1 step by step and it's quite impossible to adapt all tests at this stage. I changed only the tests which were relevant for this PR. Other tests will be updated while working on the core of the related functions, e.g. see #332.

I will screen and reply to your comments, one by one, of course.

damianooldoni and others added 3 commits September 12, 2024 10:29
No functions with such scope anymore in camtraptor.
Better for testing lifecycle deprecation warnings.
if (is.null(x) & !is.name(datapkg)) {
x <- datapkg
if (is.null(x) & !is.name(example_dataset)) {
x <- example_dataset
Copy link
Member Author

@damianooldoni damianooldoni Sep 12, 2024

Choose a reason for hiding this comment

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

Using example_dataset() is not the wished behavior.

See get_scientific_name() in main:

get_scientific_name <- function(package = NULL,
                                vernacular_name,
                                datapkg = lifecycle::deprecated()) {
check_package(package, datapkg, "get_scientific_name")
  if (is.null(package) & !is.name(datapkg)) {
    package <- datapkg
  }

The idea here was to use datapkg (deprecated) if package was not given.

Still, thanks to you I see that I have to remove this if () statement 👍
Moreover, this function will disappear in one of the next PRs. See #235.
Done in a7193bf

@sannegovaert
Copy link
Member

Thanks for addressing everything, Damiano!
I just replaced another lifecycle_warning_deprecated.
All seems good to go now!

@damianooldoni
Copy link
Member Author

Hi @sannegovaert, thanks for checking again. Can you officially approve the PR? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants