search

OHDSI / ROhdsiWebApi

An R package for interfacing with a WebAPI instance
https://ohdsi.github.io/ROhdsiWebApi
10 stars 17 forks source link

Shouldn't add 'Df' to a variable name just because it might be a data frame #36

Closed schuemie closed 4 years ago

schuemie commented 4 years ago

I think this is the only place in the OHDSI R packages where we encode the variable type in the name. I suggest calling it 'includedConcepts' instead of 'includedConceptsDf '.

Related to https://github.com/OHDSI/ROhdsiWebApi/issues/21

gowthamrao commented 4 years ago

Would this change have downstream impact on dependencies? Are we aware of other packages that might be using this function? Documentation does not explicitly state the structure of returned object https://github.com/OHDSI/ROhdsiWebApi/blob/8ed2756f727753d3d031405b633c993369c5eb18/R/CohortDefinition.R#L332

Should we work on this now - or later? i.e. part of minor release or major release - or doesnt matter, because ROhdsiWebApi is still in early stages ?

schuemie commented 4 years ago

I suggest just changing it. It is my impression that there aren't that many users, and the users that are there are often one-time users.

We should of course document the change in NEWS.md. Which we should add ;-)

gowthamrao commented 4 years ago

Will be resolved by https://github.com/OHDSI/ROhdsiWebApi/pull/64 The function is being deleted as it is still in develop branch.