Add `set_layer_coded_values()` and export `set_layer_col_names()`

elipousson commented 1 week ago

Checklist

[X] update NEWS.md
[X] documentation updated with devtools::document()
[X] devtools::check() passes locally

Changes

Export set_layer_col_names() (previously used internally by arc_read()) to allow use of alias values with data returned by arc_select() (#169).
Add new set_layer_coded_values() function to support replacement or labelling of values with coded value domains (#134).

Also add helper functions to support set_layer_coded_values():

list_field_domains()
pull_pull_coded_values()

Issues that this closes

Partly addresses this issue: https://github.com/R-ArcGIS/arcgislayers/issues/134

There may be a need for functions addressing the other type of domain values and/or validating data for consistency with set domain values before adding data to an existing layer.

Follow up tasks

Add tests for set_layer_coded_values()
Document and export list_field_domains()

elipousson commented 1 week ago

Took more work than I hoped to disentangle the col_names argument handling from the alias handling but I think you're right and it is better this way. Proposing set_layer_aliases and encode_field_values as the new function names. If you wanted to add some tests, that would be a big help.

On reflection, I also don't think list_field_domains() really need to be exported - it seems unlikely anyone who isn't a developer would need it.

JosiahParry commented 1 week ago

@elipousson arc_read() is now broken because the default argument in arc_read() is drop.

How do you want to handle this? Ideally, the default behavior is do nothing.

furl <- "[https://sampleserver6.arcgisonline.com/arcgis/rest/services/Census/MapServer/3](vscode-file://vscode-app/Applications/Positron.app/Contents/Resources/app/out/vs/code/electron-sandbox/workbench/workbench.html#)"
res <- arc_read(furl, n_max = 20)
#> Error in `arc_read()`:
#> ! `alias` must be one of "label" or "replace", not "drop".

Relatedly it looks like arc_read(furl, n_max = 20, col_names = FALSE) creates an error because the geometry column name is renamed without setting the attr(x, "sf_column") appropriately.

We should probably figure this out within this PR too—scope creep eeek!—so that we're not merging any more bugs.

Looks like the logic in set_col_names() is a bit out of order. I also think there may be some duplication or a bit of obfuscation with the repair_layer_names() and set_col_names() which might make catching these issues a bit tougher.

For example set_col_names() is defined and used only once inside of arc_read() and repair_layer_names() is used inside of set_col_names() and set_layer_aliases(). These functions are both modifying the column names and it feels a bit redundant / cluttered. Though I'm not quite sure how to tidy it up.

JosiahParry commented 1 week ago

@elipousson are you okay if i take a stab at this a bit?

elipousson commented 1 week ago

I know exactly how the bugs got introduced - I tried to account for both of them but clearly didn’t. To fix the first bug, you can validate alias with arg_match inside of arc_read before calling set_col_names. Have at it if you like or I can get to it tonight or tomorrow!

elipousson commented 1 week ago

Got it!

JosiahParry commented 3 days ago

I've gone ahead and taken the liberty to finish this PR. Note that all exported functions must have an example and also specify the @returns tag otherwise CRAN maintainers can reject the package.

I've added argument validation to set_layer_aliases(), encode_field_values(), and arc_read()
I've changed the default from "label" to replace for set_layer_aliases()—I'm still unsure what the label attribute gives you. This seems useful for advanced users who may want to play with attributes.
I've added tests for encode_field_values()
I've added tests for set_layer_aliases()
I've added tests for arc_read()

R-ArcGIS / arcgislayers