Inherit parsing issues

[damianooldoni]

This PR is quite small and is a small patch to solve a small bug noticed by @jimcasaer. While reading a datapackage, a warning about parsing issues in deployments was returned on screen. However, nothing was returned by readr::problems(dp$data$deployments).

The reason? While working on v0.20 I removed by accident the few lines of code where I assigned reading issues of deployments and media to the attributes of the corresponding data.frames.

[damianooldoni]

Improving tests at the moment...

[damianooldoni]

Still getting failing check on Ubuntu-latest (3.6.3). It's an issue with the installation of terra package.
https://github.com/inbo/camtraptor/actions/runs/6356494664/job/17288425829?pr=282#step:5:4726

@PietrH; if you agree, I would remove (temporarily?) this check. We have already a windows-latest (3.6.0) check to ensure compatibility with R 3.6.x.

[PietrH]

edit: resolved

test-read_camtrap_dp.R does print some messages to console during testing, this is because in zzz.R:

camtraptor/R/zzz.R

Lines 464 to 471 in d79224f

	message(
	writeLines(
	c(
	"The dataset uses Camtrap DP version 1.0-rc.1, it has been converted to 0.1.6.",
	"See https://inbo.github.io/camtraptor/#camtrap-dp for details."
	)
	)
	)

We use writeLines() to print to console, not warning() or message(), this slightly different approach results in suppressMessages(read_camtrap_dp()) still printing to console while testing.

[PietrH]

Still getting failing check on Ubuntu-latest (3.6.3). It's an issue with the installation of terra package. https://github.com/inbo/camtraptor/actions/runs/6356494664/job/17288425829?pr=282#step:5:4726

@PietrH; if you agree, I would remove (temporarily?) this check. We have already a windows-latest (3.6.0) check to ensure compatibility with R 3.6.x.

It seems to be fixed in the current development build of terra: rspatial/terra#1276, I didn't check on my system as I don't have R3.6.x installed at the moment.

They were using the new anonymous functions introduced in R4.1, although it surprises me that our tests passed on Windows 3.6.0

Originally I was thinking about a problem with different line breaks between Linux and Windows, similar to: https://stackoverflow.com/questions/56471638/error-unable-to-collate-and-parse-r-files-for-package-upload-to-cran

---

In principle I don't protest removing the check on Ubuntu, however, are should make sure we aren't accidentally (re-)discovering a lingering issue in terra.

[PietrH]

Some things I fixed in commits:

• moved creation of objects needed for tests to very first line of test script, is clearer to contributes (fooled me!) that these objects are created if they are running tests out of order
• replaced expect_equal(class()) with expect_type(), also consider expect_s3_class(): easier to read, less error prone
• replace expect_equal(length()) with expect_length(): easier to read
• replaced expect_equal() with expect_identical() where I felt it was a better match, increasing test strictness. expect_equal() has a built in tolerance.
• fixed Inherit parsing issues #282 (comment) : messages in helper are now properly suppressable

in test-read_camtrap_dp.R we are reusing even objects between tests, we could put those in the start of the file as well, this way the tests will run faster, and it's easier for us to make changes in the way these objects are created.

[PietrH]

One final comment:

I'm having some trouble with this test:

camtraptor/tests/testthat/test-read_camtrap_dp.R

Lines 303 to 319 in 1b749cd

	test_that("path is deprecated", {
	dp_path_warning <- system.file("extdata", "mica", package = "camtraptor")
	rlang::with_options(
	lifecycle_verbosity = "warning",
	suppressMessages(expect_warning(read_camtrap_dp(
	file = dp_path_warning,
	media = FALSE
	)))
	)
	rlang::with_options(
	lifecycle_verbosity = "warning",
	suppressMessages(expect_warning(read_camtrap_dp(
	path = dp_path_warning,
	media = FALSE
	)))
	)
	})

• Why do we suppress the messages of expect_warning?
• I can't get the deprecation warning to actually show in my console, but that might be on my end as it might only show once and then not again for a few hours.
• When running covr I ran into this error:

-- Error ('tests/testthat/test-read_camtrap_dp.R:305:3'): path is deprecated ---
Error in `lifecycle_message(when, what, with, details, env, signaller = "deprecate_warn")`: Can't detect the package of the deprecated function.
Please mention the namespace:

  # Good:
  deprecate_warn(what = "namespace::myfunction()")

  # Bad:
  deprecate_warn(what = "myfunction()")

Backtrace:
     x
  1. +-rlang::with_options(...) at tests/testthat/test-read_camtrap_dp.R:305:2
  2. +-base::suppressMessages(...)
  3. | \-base::withCallingHandlers(...)
  4. +-testthat::expect_warning(...)
  5. | \-testthat:::expect_condition_matching(...)
  6. |   \-testthat:::quasi_capture(...)
  7. |     +-testthat (local) .capture(...)
  8. |     | \-base::withCallingHandlers(...)
  9. |     \-rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
 10. \-read_camtrap_dp(file = dp_path_warning, media = FALSE)
 11.   \-lifecycle::deprecate_warn(...)
 12.     +-lifecycle:::deprecate_warn0(...)
 13.     | +-id %||% paste_line(msg)
 14.     | \-lifecycle:::paste_line(msg)
 15.     |   +-base::paste(chr(...), collapse = "\n")
 16.     |   \-rlang::chr(...)
 17.     |     \-rlang::dots_values(...)
 18.     \-lifecycle:::lifecycle_message(when, what, with, details, env, signaller = "deprecate_warn")
 19.       \-lifecycle:::spec(what, env, signaller = signaller)
 20.         \-lifecycle:::spec_pkg(what$pkg, env, ctxt = ctxt)
 21.           \-cli::cli_abort(...)
 22.             \-rlang::abort(...)

However the documentation https://search.r-project.org/CRAN/refmans/lifecycle/html/deprecate_soft.html mentions the namespace declaration as optional.

@damianooldoni , could you have a look at this test to see if to works as intended?

[PietrH]

• I made some small changes, see the comments above.
• I'm having trouble verifying if the deprecation test works as indented. Comment above.
• Don't forget to use testtthat convenience functions like expect_length and expect_type, they are easier to read and sometimes have other advantages too.
• Great job, looks great. This test is getting quite big! Thank you for tackling this

[damianooldoni]

Thanks @PietrH for your extended review and actively pushing!

@PietrH: I think we could include this work in the next release. I see you mentioned this PR in #282. As #282 is much more urgent than this patch work I would proceed as following:

1. finalize 284 add support for camtrap dp v1.0 #286 and merge it to main
2. pull changes in this branch and solve possible conflicts
3. work as much as possible on your requested changes in the tests

[damianooldoni]

Replies to #282

• Why do we suppress the messages of expect_warning?
  To avoid to print the standard message on console: "Please make sure you have the right to access data from this Data Package for your intended use. Follow applicable norms or requirements to credit the dataset and its authors. For more information, see https://doi.org/10.5281/zenodo.4893244"

I can't get the deprecation warning to actually show in my console, but that might be on my end as it might only show once and then not again for a few hours.

Yes, it's correct. However, by restarting R, you should be able to get the warning again.

When running covr I ran into this error: (about soft deprecation)
Agreed, see commit 161339c

[damianooldoni]

Thanks @PietrH for your in depth review and for improving the coding and the quality of the tests.

@peterdesmet and @PietrH: I didn't add anything on NEWS.md about this PR as it is a very small patch work. However, feel free to disagree. If so, please add please a line if you think so. Something like:

• Solve a bug in read_camtrap_dp() while returning parsing issues (see Inherit parsing issues #282).

I would first finalize the more important PR #289, then pulling the changes in main into this PR. I don't expect any conflict this time as PR #289 didn't modify anything related to read_camtrap_dp() and its tests.