316 use etnservice as a dependency and drop sql helpers #317

PietrH · 2024-09-27T07:45:26Z

update from v2.3
add patch note to NEWS
remove sql helpers
check if experimental code is replaced by more general helper

When I first started working on this, I didn't implement the changes to the helper immediately, but rather experimented in a single function: b03e88f, later on I moved this change to the forward_to_api() helper, meaning that a lot of lines can be removed. All this code still exists of course, just in etnservice instead of here as a duplicate.

From this PR onward, the API and local database connection should always return exactly the same result, any differences resulting from unintended type parsing should be considered a bug.

It seems not all the changes from v2.3 are present in this branch, namely the vignettes.

fix merge conflict
check if branch is up to date with v2.3
pass R CMD CHECK except for Test if API and local SQL queries result in same objects #330
~~remove unused dependencies -> indirect, now moved to etnservice~~ moved to issue v2.3.1: remove dependencies that have now become indirect via etnservice #331

API - SQL mismatch

mismatches for:

get_cpod_projects
list_animal_project_codes
list_acoustic_project_codes
get_animals
list_receiver_ids
get_tags
get_animal_projects
list_scientific_names
get_acoustic_projects

get_cpod_projects

[1] "get_cpod_projects"
Error: `api_result` (`actual`) not identical to `sql_result` (`expected`).

`actual$moratorium` is a character vector ('0', '1', '1', '1', '0', ...)
`expected$moratorium` is a logical vector (FALSE, TRUE, TRUE, TRUE, FALSE, ...)

list_animal_ids: missing animal ID

56314 known id is missing from both the API and SQL result, I've sent an email to Claudia

…r API calls.

…s' of github.com:inbo/etn into 316-use-etnservice-as-a-dependency-and-drop-_sql-helpers

…_sql-helpers

PietrH · 2024-10-18T08:17:52Z

Should we have a seperate test file that checks if the API and SQL return the same results? Or should these tests be included in the function test files?

I'm quite tempted to use:

for(function_to_test in getNamespaceExports("etnservice")) {
  test_that(paste(
    function_to_test,
    "returns same result over api as over local db connection"
  ),
  {
    skip_if_offline()
    
    api_result <- do.call(function_to_test, list(api = TRUE))
    sql_result <- do.call(function_to_test, list(api = FALSE))
    expect_identical(api_result, sql_result, label = function_to_test)
  })
}

But not all functions run without arguments (returned objects too large?)

peterdesmet · 2024-10-18T08:38:49Z

Maybe this is a good use case for a custom expectation.

For example, you could create a expect_call_agnostic(), with one argument: a function call. The helper could grab the function call (and its arguments), run it locally and over API and expect the result to be the same.

It could then be called as part of a regular test-function.R file:

testthat("get_animals() returns same result locally and via API", {
  expect_call_agnostic(get_animals(animal_id = "5"))
})

PietrH · 2024-10-18T08:45:48Z

That sounds great! Do you think we should include that here, or rather as a separate PR?

PietrH · 2024-10-18T08:53:53Z

9 functions currently fail to return the same result over the api as over a local connection.

peterdesmet · 2024-10-18T08:56:44Z

That sounds great! Do you think we should include that here, or rather as a separate PR?

I'll leave that to you

PietrH · 2024-10-18T11:17:02Z

Ready for review, this is a merge to the next release branch. There will be R CMD Check errors due to a mismatch between the API and SQL returned results, apart from that, it should work!

Quite a nice reduction, this will make keeping etn and etnservice in sync much much easier 🌻

Remove indirect dependencies

PietrH · 2024-10-22T08:22:13Z

I have dropped a number of dependencies and helpers that are now unused (they were moved to etnservice)

peterdesmet

Nice cleanup. Some minor suggestions in comments.

DESCRIPTION

NEWS.md

peterdesmet · 2024-10-25T13:46:05Z

R/utils.R

@@ -376,36 +323,12 @@ conduct_parent_to_helpers <- function(api,
           ...)
    )
  } else {
-    out <- do.call(glue::glue("{function_identity}_sql"), arguments_to_pass)
+    out <- do.call(utils::getFromNamespace(function_identity, ns = "etnservice"),


I would argue that some of the functions in utils.R are that big and crucial that they should be moved to their own file, e.g. conduct_parent_to_helpers.R. Doesn't necessarily need to be tackled in this PR.

I agree. How do you feel about keeping the api helpers (eg. get_val() together? They are pretty small but feel separate from other helpers (eg. check_value()).

I'm 50/50 about creating a api_helpers.R and keeping them together, but separate from other helpers so it's easier to differentiate for new readers what helper functions interact with the API only. I do test them together already in test-api_helpers.R

What do you think?

Sounds good to keep API helpers together. I would name the file utils-api.R or utils-api_helpers.R, cf. how usethis::use_pipe() creates a utils-pipe.R

https: //github.com//pull/317#discussion_r1816704543 Co-Authored-By: Peter Desmet <[email protected]>

Co-Authored-By: Peter Desmet <[email protected]>

PietrH · 2024-11-05T12:20:57Z

That sounds great! Do you think we should include that here, or rather as a separate PR?

I'll leave that to you

This is going to take more work than I want to tackle in this PR, so I'll leave it like it is here. Issue #330

https: //github.com//pull/317#discussion_r1816719227 Co-Authored-By: Peter Desmet <[email protected]>

PietrH added 8 commits September 26, 2024 16:55

Add dependency on etnservice for local database connections

f430851

pass credentials to etnservice function

b03e88f

remove unused test

d49096f

attempt to universially refer to etnservice when api is false

e12bf04

Change the way the arguments are passed so it's identical as it is fo…

2c2b499

…r API calls.

Increment version number to 2.3.1

0d8f0b5

Add patch description to NEWS

a694343

Remove SQL helpers: now use etnservice dependency instead

0e15606

PietrH added this to the v2.3.1 milestone Oct 4, 2024

Remove SQL helpers: now use etnservice dependency instead

ad753e0

PietrH force-pushed the 316-use-etnservice-as-a-dependency-and-drop-_sql-helpers branch from 0e15606 to ad753e0 Compare October 4, 2024 09:55

PietrH added 3 commits October 4, 2024 11:56

Merge branch '316-use-etnservice-as-a-dependency-and-drop-_sql-helper…

21ca37b

…s' of github.com:inbo/etn into 316-use-etnservice-as-a-dependency-and-drop-_sql-helpers

update snapshots message type

09a35f8

add skip messages

614e79d

PietrH changed the base branch from v2.3 to v2.3.1 October 18, 2024 07:09

PietrH added 2 commits October 18, 2024 09:23

Set dev version

9ad28f3

Merge branch 'v2.3' into 316-use-etnservice-as-a-dependency-and-drop-…

ea2d229

…_sql-helpers

PietrH linked an issue Oct 18, 2024 that may be closed by this pull request

Use etnservice as a dependency and drop _sql helpers #316

Open

PietrH added the API label Oct 18, 2024

PietrH added 2 commits October 18, 2024 09:37

declare utils namespace

3285d1d

update snapshots

fee2107

update casette

afdf082

This was referenced Oct 18, 2024

Test if API and local SQL queries result in same objects #330

Open

v2.3.1: remove dependencies that have now become indirect via etnservice #331

Open

PietrH marked this pull request as ready for review October 18, 2024 11:15

PietrH requested review from peterdesmet and sannegovaert October 18, 2024 11:15

PietrH added 5 commits October 22, 2024 10:02

remove unused helper -> moved to etnservice

47bf414

local database connection is made via etnservice

96805a1

remove unused helper -> moved to etnservice

21c93fe

Add utils for calling from etnservice namespace

49a2104

add utils to call from etnservice namespace

121d097

PietrH mentioned this pull request Oct 22, 2024

Remove indirect dependencies #332

Merged

Merge pull request #332 from inbo/remove-indirect-dependencies

85bf463

Remove indirect dependencies

PietrH linked an issue Oct 22, 2024 that may be closed by this pull request

v2.3.1: remove dependencies that have now become indirect via etnservice #331

Open

peterdesmet approved these changes Oct 25, 2024

View reviewed changes

PietrH and others added 3 commits November 5, 2024 13:16

last release + dev instead of version up

43433b0

https: //github.com//pull/317#discussion_r1816704543 Co-Authored-By: Peter Desmet <[email protected]>

Include change in v2.3.0 release

db50e62

Co-Authored-By: Peter Desmet <[email protected]>

Simplify

4d573bd

Move conduct_parent_to_helpers to own file

1f22adc

https: //github.com//pull/317#discussion_r1816719227 Co-Authored-By: Peter Desmet <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

316 use etnservice as a dependency and drop sql helpers #317

316 use etnservice as a dependency and drop sql helpers #317

PietrH commented Sep 27, 2024 •

edited

Loading

PietrH commented Oct 18, 2024

peterdesmet commented Oct 18, 2024

PietrH commented Oct 18, 2024

PietrH commented Oct 18, 2024

peterdesmet commented Oct 18, 2024

PietrH commented Oct 18, 2024 •

edited

Loading

PietrH commented Oct 22, 2024

peterdesmet left a comment

peterdesmet Oct 25, 2024

PietrH Nov 5, 2024

peterdesmet Nov 6, 2024

PietrH commented Nov 5, 2024

316 use etnservice as a dependency and drop sql helpers #317

Are you sure you want to change the base?

316 use etnservice as a dependency and drop sql helpers #317

Conversation

PietrH commented Sep 27, 2024 • edited Loading

API - SQL mismatch

get_cpod_projects

list_animal_ids: missing animal ID

PietrH commented Oct 18, 2024

peterdesmet commented Oct 18, 2024

PietrH commented Oct 18, 2024

PietrH commented Oct 18, 2024

peterdesmet commented Oct 18, 2024

PietrH commented Oct 18, 2024 • edited Loading

PietrH commented Oct 22, 2024

peterdesmet left a comment

Choose a reason for hiding this comment

peterdesmet Oct 25, 2024

Choose a reason for hiding this comment

PietrH Nov 5, 2024

Choose a reason for hiding this comment

peterdesmet Nov 6, 2024

Choose a reason for hiding this comment

PietrH commented Nov 5, 2024

PietrH commented Sep 27, 2024 •

edited

Loading

PietrH commented Oct 18, 2024 •

edited

Loading