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

Update read_habitatdata to version 2023 #184

Merged
merged 8 commits into from
Sep 30, 2024
Merged

Conversation

cecileherr
Copy link
Collaborator

Updated code and documentation to version 2023 for:

  • read_habitatmap
  • read_habitatmap_stdized
  • read_habitatmap_terr
  • read_habitatstreams
  • read_watersurfaces_hab

Added an attribute fix_geom to read_habitatmap (without the st_is_valid control that runs too slow and for which I haven't found a solution yet)

Copy link
Member

@florisvdh florisvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done @cecileherr, thanks a lot!! ✨

I suggest to revert f9fbf66 (which relates to #179), for the following reasons:

  • there are > 350 occurrences of .data$ in the package, so this would only be a start to solve the issue
  • the warnings only appear when editing (debugging) {n2khab} functions, not when running them. So currently it's more a developer issue than a user issue. (Still, it would become urgent once {tidyselect} would drop support for .data$, if it ever will).
  • most importantly, dropping .data$ provides not enough remediation since tidy selection is not supported by R CMD check; f9fbf66 triggers below note (which is exactly the reason why .data$ was formerly advised in R packages that use tidy selection).
── R CMD check results ─────────────────────────────── n2khab 0.10.1.9000 ────
Duration: 58.6s

❯ checking R code for possible problems ... NOTE
  read_habitatstreams: no visible binding for global variable ‘NAAM’
  read_habitatstreams: no visible binding for global variable ‘BRON’
  read_habitatstreams: no visible binding for global variable
    ‘river_name’
  read_habitatstreams: no visible binding for global variable ‘source_id’
  read_watersurfaces_hab: no visible binding for global variable
    ‘polygon_id’
  read_watersurfaces_hab: no visible binding for global variable
    ‘certain’
  Undefined global functions or variables:
    BRON NAAM certain polygon_id river_name source_id

0 errors ✔ | 0 warnings ✔ | 1 note ✖

Starting with {tidyselect} 1.2.0, tidy selection should work by quoting variable names instead of using .data$; at the same time, we also need to distinguish tidy selection from data masking (see here). We need to have this checked well in various places in {n2khab} when doing this migration, as we have no unit tests in place (yet).

R/read_habitatdata.R Outdated Show resolved Hide resolved
R/read_habitatdata.R Outdated Show resolved Hide resolved
@cecileherr
Copy link
Collaborator Author

I suggest to revert f9fbf66 (which relates to #179), for the following reasons:

commit 39f3976

@florisvdh
Copy link
Member

Perfect! 👌

@florisvdh florisvdh merged commit 60209b1 into dev_nextrelease Sep 30, 2024
3 checks passed
@florisvdh florisvdh deleted the update_2023_ch branch September 30, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants