RGeo::GeoJSON.decode no longer returns nil on invalid String input

[tie-ioki]

Hi y'all,

as of 2.2.0 the comment in RGeo::GeoJSON::Coder is no longer accurate. When using invalid String input, it used to return nil, but now raises a MultiJson::ParseError. This can easily be tested by just calling RGeo::GeoJSON.decode('foo'). The issue goes away when reverting the change from JSON.parse to MultiJson.load.

Is this behavior change intended? If so, the comment should probably be adjusted.

[BuonOmo]

It is intended to go in this direction, as we already have a mixed API between different tools in the rgeo libs. The idea is to normalize raising. With that said, the PR dedicated to this is still open. So this is a bug, but I don't think we'll fix this now but rather try to close the other PR making failing a default. WDYT @keithdoggett ?

Otherwise, if you think it is important feeling this gap for the time being, would you like to open a PR @tie-ioki ?

[tie-ioki]

Nah, the change isn't any problem, but with this being intended I'd like a little more note of it. The changelog just reads

MultiJson rather than JSON

but that implying that the comment above the method proclaiming nil is returned on error isn't correct anymore is not necessarily obvious.

[BuonOmo]

I wasn't clear sorry. The intended change was to switch to multi-json and only that at the time. This introduced the bug you spotted (raising rather than returning nil).

Today, we want a general policy of raising in case of error in RGeo's public APIs (including rgeo-geojson). So this will be a feature. And when it will be a feature, it will never return nil in case of error.

[tie-ioki]

Ah I see alright, thank you! In this case I think this issue can be closed once you move over to raising wholesale.