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

Correctly implement interface for wrapper geometry. #169

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

evetion
Copy link
Member

@evetion evetion commented Oct 5, 2024

Fixes evetion/WellKnownGeometry.jl#37.

Packages should implement the two argument interface methods for geometries, and #124 did not. In WKG I assumed I could call ncoord(trait, geom), which failed.

@evetion evetion requested a review from rafaqz October 5, 2024 13:22
@rafaqz
Copy link
Member

rafaqz commented Oct 5, 2024

Beat me to it! Tests?

@evetion
Copy link
Member Author

evetion commented Oct 5, 2024

Beat me to it! Tests?

Yeah, precisely what? Hasmethod for the two argument function and implement that in testgeometry? That way we can re-use for other packages who implementend it incorrectly (might be breaking some tests somewhere else).

@rafaqz
Copy link
Member

rafaqz commented Oct 5, 2024

I mean just calling those methods with the trait somewhere or other, but certainly add to the generic tests if that makes more sense, we don't do that enough...

@evetion
Copy link
Member Author

evetion commented Oct 6, 2024

I mean just calling those methods with the trait somewhere or other, but certainly add to the generic tests if that makes more sense, we don't do that enough...

I've added tests (which failed on master) to the generic test implementation 🙂. @rafaqz, I think this is good to go now (also to fix WKG), are you ok with merging?

@rafaqz
Copy link
Member

rafaqz commented Oct 7, 2024

Would we not just call the methods?

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.

GI.astext fails on a simple GI.Polygon
2 participants