Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removal of HeuristicOverlay changing error messages downstream #794

Closed
pramsey opened this issue Jan 11, 2023 · 5 comments
Closed

Removal of HeuristicOverlay changing error messages downstream #794

pramsey opened this issue Jan 11, 2023 · 5 comments

Comments

@pramsey
Copy link
Member

pramsey commented Jan 11, 2023

This PostGIS regression test shows a new error message after a fairly unrelated commit for the floating error handler.

https://trac.osgeo.org/postgis/ticket/5312

Also note in the old path, the exception was caught and validity checked to add to the info in the error message. This new error path seems to omit that, either accidentally or deliberately due to other changes? (I think this happened in the "heuristic overlay" code pasta, which was slated for reeducation?)

@dbaston
Copy link
Member

dbaston commented Jan 11, 2023

I would guess that it is a result of #788, not #791. Is the change in error message a problem?

@pramsey
Copy link
Member Author

pramsey commented Jan 11, 2023

Yes, you're right that's where it came from. One thing lost was the check for validity when exceptions are caught... https://github.com/libgeos/geos/pull/788/files#diff-fe4091b0d1cbe6972cabc0f5daba8f737ba035a9bb569055a16a3700987b9565L432

I'm not sure we (postgis) should be quite so concerned with error messages in general, though the existence of that test implies it exists all the way back into a lot of earlier versions, which all will need to be fixed up when this version drops, alas.

cc / @robe2

@pramsey pramsey changed the title Subtle change in failure mode after FE_* fix Removal of HeuristicOverlay changing error messages downstream Jan 11, 2023
@dbaston
Copy link
Member

dbaston commented Jan 11, 2023

which all will need to be fixed up when this version drops

I thought those tests were essentially doing assert message.startsWith(expected), so you could truncate the expected values to be "GEOS Error:" or "GEOS Error: TopologyException" and live in version-independent bliss. Maybe I'm misremembering, though.

@robe2
Copy link
Member

robe2 commented Jan 27, 2023

I'm fine with truncating the messages in postgis. Did it for some others, but I'm not seeing the error anymore in PostGIS. @pramsey did you change something? debbie is all green - https://trac.osgeo.org/postgis/ticket/5312 so I guess we could close this out or more changes need to be done?

@pramsey
Copy link
Member Author

pramsey commented Jan 27, 2023

Yes, I made the test less strict in 07d29463a26ecc47dc1fd43897d05493a3645700

@pramsey pramsey closed this as completed Jan 27, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants