search

R-ArcGIS / arcgislayers

Access ArcGIS Data and Location Services
http://r.esri.com/arcgislayers/
Apache License 2.0
47 stars 10 forks source link

Fix #175 #179

Closed elipousson closed 7 months ago

elipousson commented 7 months ago

Also implement new rbind_results() function added w/ https://github.com/R-ArcGIS/arcgisutils/issues/38

Changes

Per the discussion in draft PR: https://github.com/R-ArcGIS/arcgislayers/pull/167 this PR fixes an issue w/ returning fields not specified in fields argument and

Issues that this closes

175

Follow up tasks

Additional changes as described in draft PR.

JosiahParry commented 7 months ago

This is super clear! Thank you!

We should add some tests here:

  1. what happens when we return no fields? fields = ""
  2. testing that when we have specified fields e.g fields = c("a", "b")
  3. fields with objectid specified
  4. using fields with a Table.

@elipousson if youre comfortable with me pushing to your branch I could write the tests

elipousson commented 7 months ago

Please go ahead with the tests, @JosiahParry.

Empty strings, "", cause match_fields() to error. The arg_match function helpfully lists the permitted field names and will flag any suspected spelling errors.

99% sure fields works fine with tables as well as FeatureLayers (the check for a sf_column attribute just returns NULL in that case). Passing object ID values to fields also works just fine.

You can't select or drop the geometry column using fields which makes it a bit different than a SQL query when using sf::st_read().

There may be a few more changes we want to make when I pull in the renaming and alias assignment features that I set up for arc_read(). That could easily be exported as a standalone function so it could be used with layers accessed via arc_select() if you'd like.

elipousson commented 7 months ago

Caught one more issue - I forgot to remove the existing check for fields that was expecting a scalar input. All good now, I think.