search

MiKatt / openSTARS

open source implementation of the STARS ArcGIS toolbox
https://journals.plos.org/plosone/article?id=10.1371/journal.pone.0239237
Other
46 stars 13 forks source link

rgrass7 #2

Closed jsta closed 6 years ago

jsta commented 6 years ago

This looks like a very nice package! Can I ask if there is a reason why rgrass7 is not imported in the DESCRIPTION or imported in roxygen tags? Would pull requests adding these be welcome?

MiKatt commented 6 years ago

So far, the package depends on rgrass7. However, importing it might indeed be the safer way; thanks for the hint. Feel free to create a pull request.

jsta commented 6 years ago

Ok, I hope I caught all the rgrass7 functions called from the code (execGRASS, readVECT, writeVECT) in importFrom statements.

MiKatt commented 6 years ago

As far as I know there are no ambiguities regarding the function names from rgrass7. So, for sake of readability I would prefer to skip rgrass7::. Additionally, rgrass7 consists of only a few functions so it does not harm to completely import it (instead of importFrom). Then, in the ReadMe example there would also be no need to library(rgrass7). Would you like to change this?

jsta commented 6 years ago

As far as I know, the only way to call initGRASS without library(rgrass7) is to keep rgrass7 in the Depends field in the DESCRIPTION file. If you want this behavior I think this PR should be closed?

I think what makes it tricky for me is that I am not a user of the data.table package. As a result, I have trouble being sure which which functions are rgrass7 and which are data.table without help from importFrom or ::.

jsta commented 6 years ago

Alternatively, calls to initGRASS could be moved to setup_grass_environment?

MiKatt commented 6 years ago

I thought about this option but it would mean to hide a lot of settings as function parameters. Therefore, I decided to keep it separate to keep the user aware that she is using GRASS. Thanks for your contribution anyway!