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

Updated set.rs docs to be more consistent with those in predicates.rs. #394

Merged
merged 4 commits into from
May 9, 2023

Conversation

metasim
Copy link
Contributor

@metasim metasim commented May 7, 2023

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

image

Context: #379 (comment)

/// `Some(geometry)` if both Geometries contain pointers.
/// `None` if either geometry is missing the GDAL pointer, or there is an error.
/// * `Some(geometry)`: a new `Geometry` representing the computed union
/// * `None`: when the union was not able to be computed
Copy link
Contributor Author

@metasim metasim May 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really frustrated by this, as the C/C++ docs say:

a new geometry representing the union or NULL if an error occurs.

which would imply to me that we should be returning Result<Geometry, GDALError>, except I'm not finding any evidence that an error message is ever posted.

Copy link
Member

@lnicola lnicola May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a moot point. I suspect it's impossible to get a GEOS-less GDAL unless you're compiling it yourself, but then not everyone might know that GEOS is important to have when you're using vector data.

Also, I wonder if we should mention SFCGAL, which is required for some geometry types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

/// `Some(geometry)` if both Geometries contain pointers
/// `None` if either geometry is missing the GDAL pointer, or there is an error.
/// * `Some(geometry)`: a new `Geometry` representing the computed intersection
/// * `None`: when the geometries do not intersect or result could not be computed
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along with this, frustrated to see the overloading of Option to handle both the empty geometry case as well as the error case. Not sure if we should try to fix this on the Rust side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the geometries do not intersect

At least GEOS (didn't test with SFCGAL) returns an empty geometry (not NULL) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for testing that!!

src/vector/ops/set.rs Outdated Show resolved Hide resolved
src/vector/ops/set.rs Outdated Show resolved Hide resolved
src/vector/ops/set.rs Outdated Show resolved Hide resolved
metasim and others added 3 commits May 9, 2023 13:04
Co-authored-by: Laurențiu Nicola <[email protected]>
Co-authored-by: Laurențiu Nicola <[email protected]>
Co-authored-by: Laurențiu Nicola <[email protected]>
@metasim
Copy link
Contributor Author

metasim commented May 9, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented May 9, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 4ef3cbe into georust:master May 9, 2023
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

Successfully merging this pull request may close these issues.

None yet

2 participants