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.

Sign up for GitHub

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

Jump to bottom

add Aqua testing #125

Merged
merged 4 commits into from
Jul 25, 2022
Merged

add Aqua testing #125

merged 4 commits into from
Jul 25, 2022

Conversation

visr
Copy link
Member

@visr visr commented Jul 24, 2022

Some aftercare after merging #117, @evetion could you review?

Since we are about to have a breaking release, let's remove the old deprecated functions.
Adding https://github.com/JuliaTesting/Aqua.jl testing helped found some method ambiguities that are now resolved.

visr added 4 commits July 24, 2022 21:42
@visr
remove functions that were deprecated 4 years ago
dab1945
@visr
add aqua automated quality assurance
ee7f4f3
@visr
resolve 8 method ambiguities
a0df6fc 
As found by Aqua. These come from GeoInterface fallbacks, and are all similar to:
```
Ambiguity #4
ngeom(::GeoInterface.AbstractGeometryTrait, geom::LibGEOS.Polygon)
     @ LibGEOS D:\visser_mn\.julia\dev\LibGEOS\src\geo_interface.jl:26
ngeom(::GeoInterface.AbstractPointTrait, geom)
     @ GeoInterface d:\visser_mn\.julia\packages\GeoInterface\J298z\src\fallbacks.jl:22

Possible fix, define
  ngeom(::GeoInterface.AbstractPointTrait, ::LibGEOS.Polygon)
```
@visr
run JuliaFormatter
a3ba201
@visr visr requested a review from evetion July 24, 2022 20:19
@evetion
Copy link
Member

evetion commented Jul 25, 2022

I'm ok with merging this if it makes Aqua happy.

However, I'd say it's unnecessary to define GeoInterface.ngeom(::AbstractPointTrait, geom::Polygon) = 0 (and other geometries) after GeoInterface.ngeom(::AbstractPointTrait, geom::AbstractGeometry) = 0 has been defined. And these ambiguities would only trigger if we made a mistake in deriving the correct trait for LibGEOS geometries.

@visr
Copy link
Member Author

visr commented Jul 25, 2022

And these ambiguities would only trigger if we made a mistake in deriving the correct trait for LibGEOS geometries.

That's right. Not sure if there are additional benefits to resolving these if they are not a problem in practice. We could also exclude certain measures. https://juliatesting.github.io/Aqua.jl/dev/#Aqua.test_ambiguities-Tuple{Any}
But for now I'll go ahead and release this.

@visr visr merged commit 4d0b59b into master Jul 25, 2022
@visr visr deleted the aqua branch July 25, 2022 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evetion evetion Awaiting requested review from evetion

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@visr @evetion