-
Notifications
You must be signed in to change notification settings - Fork 196
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
Falliable Boolean Operations #1032
Conversation
Since this is a stop-gap until we can implement a more bug-free implementation, could we just catch the panic and error in the |
I'm not entirely sure if I understand you correctly but I'll give it a try to answer you. Feel free to correct me:
I'm using the functions in with a
The PR is currently just a PoC draft and if we really want to merge it in I would try to make the implementation more parallel to the existing code, i.e. duplicate existing code and just "resultify" it so it's easier to remove in the future. |
Oh, good point, I didn't think about the wasm target; the impl. looks on track otherwise . Don't mind copying the algo, we can always do some git churn to revert back components. Would be good to hear from other reviewers on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on board with the idea of returning runtime errors where users are hitting panics, but I'd like to make sure we're not overly convoluting the code. See my comments on the GeoError enum.
@@ -40,6 +40,7 @@ rand_distr = "0.4.3" | |||
|
|||
### boolean-ops test deps | |||
wkt = "0.10.1" | |||
serde_json = "1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use WKT for your test fixtures? We already have the dependency and IMO it's easier to read as a human.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes total sense. I'll change that 👍
@@ -37,6 +37,16 @@ impl<F: GeoFloat> Closest<F> { | |||
} | |||
} | |||
|
|||
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | |||
pub enum GeoError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling this GeoError
is a bit aspirational since they are only for boolean ops. Is it actually useful to distinguish between all these cases? (genuine question)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that isn't final yet 😅 I just figured it would be a good start to test it all out.
I'm totally on board with you in creating a more specialized error enum there!
The distinction between all the cases was made so that we would basically still have some rough information in the normal boolops code. Since we're just unwrapping there we will at least see the error enum variant which was chosen to mirror the previous error as close as possible.
SegmentAlreadyFoundInActiveSet(usize), | ||
ExpectedNonemptyEvents, | ||
NotEulierian(usize), | ||
Unreachable(&'static str), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to put my cards on the table, I think writing panics into our code is reasonable if the programmer truly expects the code to be unreachable. And I think that trying to turn every single panic into a runtime error is probably an anti-pattern.
My preference is that this be only the set of run time errors we're actually encountering. Is it the case that you're hitting all of these?
If we are actually encountering one of the "Unreachable" cases, it's not unreachable. Can you think of a better name that might help people understand what's going on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, unfortunately I was able to trigger the unreachable code (see the 3rd case in the spoiler of this comment).
Again, I agree here as well. We should probably pick a better name here. I'll look into that. I wasn't diving too deep into that code yet since I just wanted to have some kind of parity to the existing panicing implementation, but I guess now's a good time for that.
@@ -37,6 +37,16 @@ impl<F: GeoFloat> Closest<F> { | |||
} | |||
} | |||
|
|||
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | |||
pub enum GeoError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd guess we want this to be non-exhaustive so we can add to it without breaking peoples code. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really getting what you meant here. Is it showing the right code here?
51cb2ac
to
f7e0c2f
Compare
A friend of mine found another major point of failure of the normal I'm not really sure how to fix that. We can potentially just make it not panic in the |
776367b
to
810e230
Compare
@@ -37,6 +37,16 @@ impl<F: GeoFloat> Closest<F> { | |||
} | |||
} | |||
|
|||
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | |||
pub enum GeoError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd guess we want this to be non-exhaustive
I mean like this:
pub enum GeoError { | |
#[non_exhaustive] | |
pub enum GeoError { |
That way, if we need to add new error cases in a subsequent release, we can do so without breaking existing user's code.
CHANGES.md
if knowledge of this change could be valuable to users.The idea comes from here.
tldr: I think it would be best to resultify every panic that could happen in the
BooleanOps
trait as a short term solution. This givesgeo
users the chance to catch and deal with these scenarios (e.g. adjust the geometry slightly and try again) or to ignore them if the results aren't too important.Some things to discuss
The current state of this PR is merely a PoC for now. There are some things I would like to discuss first
Error handling
Most of the errors are just placeholders for the first iteration of this PR. We can use
thiserror
to make error handling more robust but I didn't want to go that route yet without any second opinion.Breaking changes
I tried to avoid breaking changes as much as possible for now. Technically I probably did create some breaking changes which could be ironed out easiliy. I just didn't want to create a copy of all of the existing code parts yet. If you want a really clean solution, we can just create the falliable version of everything completely detached in separate functions from the current state of
geo
Todos