GSA / ckanext-geodatagov

data.gov extension
Other
35 stars 39 forks source link

Spatial plus sign fix #249

Closed btylerburton closed 1 year ago

btylerburton commented 1 year ago

Pull Request

Related to https://github.com/GSA/data.gov/issues/3549

About

PR TASKS

nickumia-reisys commented 1 year ago

How does removing the + make this work now? What about - signs?

jbrown-xentity commented 1 year ago

How does removing the + make this work now? What about - signs?

We found that our code wasn't handling + signs correctly, although the - are required for negative lat/long values. The + is pretty atypical (and is definitely not a part of the geojson spec, which expects an actual JSON number type), but it's way easier for us to handle this use case than to try to get the data providers to change how they are providing the data...

nickumia-reisys commented 1 year ago

How does removing the + make this work now? What about - signs?

We found that our code wasn't handling + signs correctly, although the - are required for negative lat/long values. The + is pretty atypical (and is definitely not a part of the geojson spec, which expects an actual JSON number type), but it's way easier for us to handle this use case than to try to get the data providers to change how they are providing the data...

That makes sense. You're saying that whatever library couldn't convert the string "+23" to the number 23. Okay.. There weren't any other issues with format parsing? It seems like this fix is one that should be handled differently. I think the problem stems from there not being a spec about this. But it should be a validation error and if we want to allow it anyway, there should be a connection between the validation failing and it entering this logic. But that's definitely out of the scope of this effort...

btylerburton commented 1 year ago

Some further edification from the spec: https://stackoverflow.com/questions/26667313/does-json-allow-positive-sign-for-numbers

nickumia-reisys commented 1 year ago

For clarity, the internal variable in the translate_spatial function was never actually modifying the true "old_spatial" variable. The change of naming helps to highlight that, so overall, great reviewing, but there was no functionality at stake.