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

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

Closed
damianooldoni opened this issue Jun 18, 2018 · 7 comments
Assignees
Labels
bug Something isn't working database Related to ETN database

Comments

@damianooldoni
Copy link
Member

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 😢

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!

@damianooldoni damianooldoni added bug Something isn't working database Related to ETN database labels Jun 18, 2018
@stijnvanhoey
Copy link
Contributor

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
Copy link
Collaborator

jreubens commented Jul 3, 2018

@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
Copy link

bwydoogh commented Jul 4, 2018

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

Done.

@stijnvanhoey
Copy link
Contributor

stijnvanhoey commented Jul 26, 2018

@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
Copy link
Member Author

@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
Copy link
Collaborator

jreubens commented Jan 4, 2019

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

@bwydoogh
Copy link

bwydoogh commented Jan 4, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working database Related to ETN database
Projects
None yet
Development

No branches or pull requests

4 participants