agrc / masquerade

Disguise UGRC's Web API endpoints as an Esri locator service.

https://github.com/agrc/masquerade#readme

MIT License

1 stars 0 forks source link

Add all SGID attributes to matched record results #27

Closed stdavis closed 3 years ago

stdavis commented 3 years ago

Closes #26

Here are some examples of how the data looks in Pro:

Before:

After:

codecov[bot] commented 3 years ago

Codecov Report

Merging #27 (8563737) into staging (7f6512a) will increase coverage by 0.37%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           staging      #27      +/-   ##
===========================================
+ Coverage    93.19%   93.56%   +0.37%     
===========================================
  Files            3        3              
  Lines          191      202      +11     
  Branches        20       21       +1     
===========================================
+ Hits           178      189      +11     
  Misses          10       10              
  Partials         3        3

Impacted Files	Coverage Δ
src/masquerade/providers/open_sgid.py	`96.74% <100.00%> (+0.31%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7f6512a...8563737. Read the comment docs.

steveoh commented 3 years ago

On the surface my graphql voice says that it's sending more data than necessary. My rational voice says it's probably not an issue since it's a slow interval request. I might recommend putting the "extra" data in an attributes dictionary to match a graphic more closely but I don't know how that would appear in pro.

stdavis commented 3 years ago

On the surface my graphql voice says that it's sending more data than necessary.

I had the same thought but in the end, decided that it's not that big of a deal since it will always be only a single feature.

I might recommend putting the "extra" data in an attributes dictionary to match a graphic more closely but I don't know how that would appear in pro.

Pro expects it in the root attributes prop as it is right now. I figured out the shape of the expected response by looking at the data that their world geocoder returned (which in most instances is WAY more fields than we would be returning from Open SGID).

stdavis commented 3 years ago

Wait, @steveoh, did you not approve this PR yet? I may have missed that. Let me know if there are any other concerns you have.

steveoh commented 3 years ago

I didn't look at any code but i'm sure it's great!

eneemann commented 3 years ago

I didn't have anything to add, I think the additional attributes will be nice!