Spatial plus sign fix

[btylerburton]

Pull Request

Related to GSA/data.gov#3549

About

PR TASKS

• The actual code changes.
• Tests written and passed.
• Any changes to docs?
• Bumped version number in setup.py (also checked on PyPi).

[nickumia-reisys]

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

[jbrown-xentity]

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...

[jbrown-xentity]

Thanks for working on this @btylerburton , one small change I think!

[jbrown-xentity]

I think we want to try to leave old_spatial as is, as that is the value given to us by the provider. I would rather see this change down near line 480, as the start of the transformation we do in that specific use case.

[nickumia-reisys]

I agree with @jbrown-xentity (having more context on this now)

[btylerburton]

I had chained the fix into line 481 old_spatial.strip().split(',') but then I realized that old_spatial is getting loaded in at L467 too and that we need to transform it before that.

[btylerburton]

Is anyone clear where initial spatial is getting saved into old_spatial? If I can find that I can ensure the initial value gets saved unchanged.

[nickumia-reisys]

Although I don't like what I'm about to suggest, but the replace function in python is a pass-by-value function, so there's technically no issue saving the result to a new variable. It creates more mess in the code, but I think that's what you need for it to load into json and then do the real work...

[nickumia-reisys]

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]

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

[nickumia-reisys]

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.

[nickumia-reisys]

🚃