florisvdh
commented
3 years ago would it be useful to adapt the letter case of the river names (some are all uppercase, others only start with a capital letter)?
Sorry, forgotten to address this (and other opinions welcome of course).
Yes, I think that would be nice indeed. Since this is a function returning the raw data source in some standard format, we don't throw away any records, nor drop actual 'data' columns unless duplicated or irrelevant, nor do we interpret or change the meaning of data. But we still do drop unneeded columns (e.g. length/area if the data is spatial already, or unneeded administrative IDs), we standardize column names, and yes, if data values can be formatted in a better way, that's certainly a nice thing to add.
If you like to add that here, you can always ask a re-review afterwards with the circular-arrows icon at the top right (
).
Please note I have not added any version control yet on purpose (technically not needed to use v1.7, and I suppose the priority is to make the new layers available to the users - we can tackle the version control at a later stage)
Yes, I follow that too, version handling will need to be reconsidered anyway. We could still have added a pro-forma argument version (I had been considering it for #115 but dropped the local commit eventually) and set it to the latest version code by default, but of course we don't use it in this case. Actually such a dummy was added in read_watersurfaces() (even as a reason for releasing n2khab 0.3.1), but there it serves to make clear (in the usage section) that the function is not yet adapted to also work with the latest version (which it can't, at least because of the different filepath: shapefile vs geopackage). It is needed when it actually impacts the function's behaviour.
I have not been too consistent, see below: watercourse_100mseg and habitatquarries have only one version too. There's a possibility that future version handling will not go through a version argument, so shouldn't bother too much, but you can choose what you like.
$ find . -type f -name *.R -exec grep -nH " version =" {} --colour \;
./R/read_habitatdata.R:122: version = "habitatmap_stdized_2018_v2"){
./R/read_habitatdata.R:295: version = "watersurfaces_hab_v3"){
./R/read_habitatdata.R:464: version = "watersurfaces_v1.0") {
./R/read_habitatdata.R:847: version = "habitatmap_terr_2018_v2"){
./R/read_habitatdata.R:1178: version = "habitatsprings_2020v2"){
./R/read_habitatdata.R:1367: version = "habitatquarries_2020v1"){
./R/read_watercourses.R:105: version = "watercourse_100mseg_20200807v1"){
cecileherr
I made a preliminary controle within n2khab-preprocessing (html notebook or script soon under src/miscellaneous): since the structure of version 1.7 has not changed at all since version 1.6, we do not need to adapt to the script itself.
Please note I have not added any version control yet on purpose (technically not needed to use v1.7, and I suppose the priority is to make the new layers available to the users - we can tackle the version control at a later stage)
Question to users: would it be useful to adapt the letter case of the river names (some are all uppercase, others only start with a capital letter)?
This should be a solution for issue #106