search

inbo / etn

R package to access data from the European Tracking Network
https://inbo.github.io/etn/
MIT License
6 stars 5 forks source link

Conflict in functions due to new column called "project_type" in view/database #59

Closed damianooldoni closed 5 years ago

damianooldoni commented 6 years ago

While starting solving issue #24 I found that many functionalities of package etn start crashing or not returning the desired result. After debugging @stijnvanhoey and I are both very sure that the problem is the introduction of a new column called project_type. Using such a name is quite unlucky because it is also the name of input parameter for filtering on animal or network projects. And dplyr doesn't like this at all unfortunately :cry:

I see two options:

  1. change of the column name in views/database
  2. change the name of parameter internally in all corrupted functions

In order to prevent this kind of bugs in the future, could @bwydoogh, @jreubens, the unit-tests perform in RStudio before any update of views/database? It is just one line of code: 

devtools::test()

If no errors are returned, then the change is welcome!

stijnvanhoey commented 6 years ago

PR #60 fixes the issue in the R code, however the unit test check when changing views in the database would be a valid part of the workflow.

Moreover, the projects view now has a column type (i.e. animal of network) and another column project_type... @jreubens are these the preferred names and will users understand the concept/difference of both names?

jreubens commented 6 years ago

@stijnvanhoey After discussion with @bwydoogh, we decided to rename the column project_type into context_type (we will let you know when that field has been renamed). The allowed values for that field are (currently): acoustic_telemetry and cpod. At the same time we believe that this ETN package should not return data within that cpod context. So, is it possible to filter the data on acoustic_telemetry only (and to avoid confusion the user should not see the field context_type in the result)?

bwydoogh commented 6 years ago

we decided to rename the column project_type into context_type (we will let you know when that field has been renamed)

Done.

stijnvanhoey commented 6 years ago

@damianooldoni also, here; should we add this as a unit-test (maybe with ref to the issue and some context description about the naming, as a kind of 'minimal' documentation? As far as I know, there is no real/specific documentation about the provided dbase views...

damianooldoni commented 6 years ago

@stijnvanhoey : yes, I agree. I don't think a unit-test is the best place where to get context description or minimal documentation. A reference to the issue in unit-test seems good. This change in column name could be otherwise added in general documentation/vignette. And maybe in description of the (related) functions as well?

jreubens commented 5 years ago

Does we need to do anything here? Or can this issue be closed?

bwydoogh commented 5 years ago

@jreubens I guess so. Issue is assigned to you and myself (and I renamed the field, so "job done").