None exception is raised when the point coordinates are invalid

[lucasgomide]

Hi, everyone!

Why there is a rescue exception when a request to generate a point fails? Should we raise an exception If the provided coordinates are not valid?

rgeo-geojson/lib/rgeo/geo_json/coder.rb

Line 198 in 7316c96

@geo_factory.point(*point_coords[0...@num_coordinates].map(&:to_f)) rescue nil

[keithdoggett]

I'm not sure. It might be related to the fact that point will throw different errors than the rest of the geometry types. @BuonOmo do you have any thoughts on this? Very possible it might not be needed.

Edit: I suppose checking the commits associated with it could provide some insight.

[BuonOmo]

@keithdoggett you're right about the commit checking part. I've already done it in some other places, and I think the only reason for this rescue is that at some point either Tee Parham or Daniel Azuma wanted to go nil on errors. However, since both raising and the nil pattern are co-existing, this just makes it harder to understand.

I would roll back on letting the factory raise IMHO.

@lucasgomide would you have time to do some commit history check on that one and ensure us it is ok ? 🙂

[lucasgomide]

@BuonOmo I got your point. I've already checked the history commits before. Although I was not able to get the reason. The first commit has already the rescue nil code (it means transfer repo).

I believe that errors are different from exceptions. It is really weird return nil when the @geo_factory.point raises an exception.

Thus, I agree with you about letting the factory raise. We could improve the errors from the factory even

[BuonOmo]

@keithdoggett should we wait for your mail to Daniel before acting here ?

[dazuma]

So when I wrote RGeo initially, I used the "return nil" pattern for all errors. In hindsight, that was a bad decision that I deeply regret, and for a long time it was my intention to change it to raise exceptions instead. Unfortunately, around that time I changed jobs, no longer worked in geospatial applications, and so wasn't able to maintain the libraries anymore. But it's still been my hope that the error reporting strategy could be changed, and I would very much support such a project.

I would just have a couple of cautionary recommendations.

1. If we make the change, make sure it's consistent and complete. The last thing we want is for some errors to return nil and others to raise exceptions. And this applies across the various libraries as well (i.e. rgeo and rgeo-geojson should behave the same.)
2. Remember this is a fundamentally breaking change. Make sure it gets handled appropriately. One idea I considered was to make the "error handling strategy" a setting/option on the factory. It could default to the current strategy of returning nil to maintain backward compatibility with existing usage, but could be optionally set to raise exceptions instead. (But don't do that just because I said so. It may be better at this point just to make the change and release a semver-major update.)
3. It would probably be a good idea to define a common exception hierarchy that applies to all factories. We don't want, for example, CAPI to raise one exception class while simple cartesian raises a different exception class.

[BuonOmo]

Thanks @dazuma !

If we make the change, make sure it's consistent and complete. The last thing we want is for some errors to return nil and others to raise exceptions. And this applies across the various libraries as well (i.e. rgeo and rgeo-geojson should behave the same.)

Unfortunately nowadays it has already a bit of unconsistancy. Yet, you're right, we should only include that change in a new major for sure.

Remember this is a fundamentally breaking change. Make sure it gets handled appropriately. One idea I considered was to make the "error handling strategy" a setting/option on the factory. It could default to the current strategy of returning nil to maintain backward compatibility with existing usage, but could be optionally set to raise exceptions instead. (But don't do that just because I said so. It may be better at this point just to make the change and release a semver-major update.)

I'd vote for a opiniated breaking change since RGeo has already a lot of options. What do you think @keithdoggett ?

It would probably be a good idea to define a common exception hierarchy that applies to all factories. We don't want, for example, CAPI to raise one exception class while simple cartesian raises a different exception class.

Of course that would be sensible. This means reverting a kind of error I introduced in a recent PR (GeosError), but I think that is a good point. IMHO we may allow specific errors only if they are subclasses of the common errors. (for instant there could be a UnsupportedGeosVersionError which comes from RGeoError)

[keithdoggett]

Thanks for the notes @dazuma

@BuonOmo my initial thoughts align with yours that we should just make a breaking change, but I'd like to spend more time thinking about it.

Once we're ready to discuss validation in general, we can plan out an exception hierarchy and then do a deep dive through the ecosystem and try to find existing inconsistencies.