Add Aqua tests and resolve method ambiguities

Project.toml

@@ -38,12 +38,13 @@ julia = "1.9"
 
 [extras]
 ArchGDAL = "c9ce4bd3-c3d5-55b8-8973-c0e20141b8c3"
+Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595"
 DataFrames = "a93c6f00-e57d-5684-b7b6-d8193f3e46c0"
 Makie = "ee78f7c6-11fb-53f2-987a-cfe4a2b5a57a"
 Plots = "91a5bcdd-55d7-5caf-9e0b-520d859cae80"
 RemoteFiles = "cbe49d4c-5af1-5b60-bb70-0a60aa018e1b"
-ZipFile = "a5390f91-8eb1-5f08-bee0-b1d1ffed6cea"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
+ZipFile = "a5390f91-8eb1-5f08-bee0-b1d1ffed6cea"
 
 [targets]
-test = ["ArchGDAL", "DataFrames", "Makie", "Plots", "RemoteFiles", "ZipFile", "Test"]
+test = ["ArchGDAL", "Aqua", "DataFrames", "Makie", "Plots", "RemoteFiles", "ZipFile", "Test"]

ext/ShapefileMakieExt.jl

@@ -10,7 +10,7 @@ GeoInterfaceMakie.@enable Shapefile.LinearRing
 Makie.plottype(tbl::Shapefile.Table) = Makie.plottype(Shapefile.shapes(tbl))
 Makie.plottype(shp::Shapefile.Handle) = Makie.plottype(Shapefile.shapes(shp))
 
-for T in (Any, Union{Type{Any}, Type{<:Makie.AbstractPlot}}, Type{<:Makie.Poly}, Type{<:Makie.Lines}, Makie.PointBased)
+for T in (Makie.ConversionTrait, Type{<:Makie.AbstractPlot}, Type{<:Makie.Poly}, Type{<:Makie.Lines}, Makie.PointBased)
     @eval begin
         Makie.convert_arguments(t::$T, tbl::Shapefile.Table) =
             Makie.convert_arguments(t, Shapefile.shapes(tbl))

src/polygons.jl

@@ -35,10 +35,9 @@ struct SubPolygon{L<:LinearRing} <: AbstractVector{L}
 end
 GI.isgeometry(::Type{<:SubPolygon}) = true
 GI.geomtrait(::SubPolygon) = GI.PolygonTrait()
-GI.is3d(::GI.PolygonTrait, p::SubPolygon) = GI.is3d(first(p))
-GI.ismeasured(::GI.PolygonTrait, p::SubPolygon) = GI.measures(first(p))
 GI.ncoord(::GI.PolygonTrait, ::SubPolygon{<:LinearRing{P}}) where {P} = _ncoord(P)
 GI.ngeom(::GI.PolygonTrait, sp::SubPolygon) = length(sp)
+# `is3d` and `ismeasured` are defined in `extents.jl`
 Base.@propagate_inbounds GI.getgeom(::GI.PolygonTrait, sp::SubPolygon, i::Integer) =
     getindex(sp, i)
 

test/runtests.jl

@@ -3,8 +3,11 @@ using Shapefile
 using GeoFormatTypes
 using Test
 
+import Aqua
+
 const GI = GeoInterface
 
+import GeoInterfaceRecipes # for ambiguity ignore list only
 
 using Shapefile: Point, PointM, PointZ, Polygon, PolygonM, PolygonZ, Polyline,
     PolylineM, PolylineZ, MultiPoint, MultiPointM, MultiPointZ,
@@ -133,125 +136,144 @@ test_shapes = Dict(
     SubPolygon => SubPolygon([LinearRing{Point}(view(points, 1:4))]),
 )
 
-@testset "GeoInterface compatibility" begin
-    foreach(test_shapes) do (k, s)
-        @test GeoInterface.testgeometry(s)
-        @test GeoInterface.extent(s) isa GeoInterface.Extent
-        # This test should be moved to GeoInterface `testgeometry`
-        @test GeoInterface.ncoord(s) ==
-            (2 + GeoInterface.is3d(s) + GeoInterface.ismeasured(s)) ==
-            length(GeoInterface.coordnames(s))
-        # TODO Plotting a single measured point fails, needs fixing in GeoInterfaceRecipes.jl
-        GeoInterface.ismeasured(s) && GeoInterface.trait(s) isa GeoInterface.PointTrait || plot(s)
-    end
-    @test_broken GeoInterface.testgeometry(MultiPatch(Rect(1, 3, 2, 4), [0], [1], points, Interval(1, 4), [1, 2, 3, 4]))
+@testset "Shapefile.jl" begin
+
+    @testset "GeoInterface compatibility" begin
+        foreach(test_shapes) do (k, s)
+            @test GeoInterface.testgeometry(s)
+            @test GeoInterface.extent(s) isa GeoInterface.Extent
+            # This test should be moved to GeoInterface `testgeometry`
+            @test GeoInterface.ncoord(s) ==
+                (2 + GeoInterface.is3d(s) + GeoInterface.ismeasured(s)) ==
+                length(GeoInterface.coordnames(s))
+            # TODO Plotting a single measured point fails, needs fixing in GeoInterfaceRecipes.jl
+            GeoInterface.ismeasured(s) && GeoInterface.trait(s) isa GeoInterface.PointTrait || plot(s)
+        end
+        @test_broken GeoInterface.testgeometry(MultiPatch(Rect(1, 3, 2, 4), [0], [1], points, Interval(1, 4), [1, 2, 3, 4]))
 
-    @test GeoInterface.extent(Shapefile.Point(1.0, 2.0)) == Extents.Extent(X=(1.0, 1.0), Y=(2.0, 2.0))
-    @test GeoInterface.extent(Shapefile.PointM(1.0, 2.0, 4.0)) == Extents.Extent(X=(1.0, 1.0), Y=(2.0, 2.0))
-    @test GeoInterface.extent(Shapefile.PointZ(1.0, 2.0, 3.0, 4.0)) == Extents.Extent(X=(1.0, 1.0), Y=(2.0, 2.0), Z=(3.0, 3.0))
-    @test GeoInterface.extent(test_shapes[Polygon]) == Extents.Extent(X=(1.0, 2.0), Y=(3.0, 4.0))
-    @test GeoInterface.extent(test_shapes[PolygonM]) == Extents.Extent(X=(1.0, 2.0), Y=(3.0, 4.0))
-    @test GeoInterface.extent(test_shapes[PolygonZ]) == Extents.Extent(X=(1.0, 2.0), Y=(3.0, 4.0), Z=(1.0, 4.0))
+        @test GeoInterface.extent(Shapefile.Point(1.0, 2.0)) == Extents.Extent(X=(1.0, 1.0), Y=(2.0, 2.0))
+        @test GeoInterface.extent(Shapefile.PointM(1.0, 2.0, 4.0)) == Extents.Extent(X=(1.0, 1.0), Y=(2.0, 2.0))
+        @test GeoInterface.extent(Shapefile.PointZ(1.0, 2.0, 3.0, 4.0)) == Extents.Extent(X=(1.0, 1.0), Y=(2.0, 2.0), Z=(3.0, 3.0))
+        @test GeoInterface.extent(test_shapes[Polygon]) == Extents.Extent(X=(1.0, 2.0), Y=(3.0, 4.0))
+        @test GeoInterface.extent(test_shapes[PolygonM]) == Extents.Extent(X=(1.0, 2.0), Y=(3.0, 4.0))
+        @test GeoInterface.extent(test_shapes[PolygonZ]) == Extents.Extent(X=(1.0, 2.0), Y=(3.0, 4.0), Z=(1.0, 4.0))
 
-    subpolygon = getgeom(test_shapes[Polygon], 1)
-    @test GeoInterface.testgeometry(subpolygon)
-    linearring = getgeom(subpolygon, 1)
-    @test GeoInterface.testgeometry(linearring)
-    @test GeoInterface.extent(subpolygon) == Extents.Extent(X=(1.0, 2.0), Y=(3.0, 4.0))
+        subpolygon = getgeom(test_shapes[Polygon], 1)
+        @test GeoInterface.testgeometry(subpolygon)
+        linearring = getgeom(subpolygon, 1)
+        @test GeoInterface.testgeometry(linearring)
+        @test GeoInterface.extent(subpolygon) == Extents.Extent(X=(1.0, 2.0), Y=(3.0, 4.0))
 
-    @test GeoInterface.trait(shp) isa GeoInterface.GeometryCollectionTrait
-    @test GeoInterface.testgeometry(shp)
-end
+        @test GeoInterface.trait(shp) isa GeoInterface.GeometryCollectionTrait
+        @test GeoInterface.testgeometry(shp)
+    end
 
-@testset "Loading Shapefiles" begin
+    @testset "Loading Shapefiles" begin
 
-for test in test_tuples
-    for shx in (:manual, :auto, :none)
-        shp = if shx == :manual
-            # this accesses .shp based on offsets in .shx
-            stempath, ext = splitext(test.path)
-            shxname = string(stempath, ".shx")
-            Shapefile.Handle(joinpath(@__DIR__, test.path), joinpath(@__DIR__, shxname))
-        elseif shx == :auto
-            Shapefile.Handle(joinpath(@__DIR__, test.path))
-        else
-            # use .shp only
-            open(joinpath(@__DIR__, test.path)) do fd
-                read(fd, Shapefile.Handle)
+    for test in test_tuples
+        for shx in (:manual, :auto, :none)
+            shp = if shx == :manual
+                # this accesses .shp based on offsets in .shx
+                stempath, ext = splitext(test.path)
+                shxname = string(stempath, ".shx")
+                Shapefile.Handle(joinpath(@__DIR__, test.path), joinpath(@__DIR__, shxname))
+            elseif shx == :auto
+                Shapefile.Handle(joinpath(@__DIR__, test.path))
+            else
+                # use .shp only
+                open(joinpath(@__DIR__, test.path)) do fd
+                    read(fd, Shapefile.Handle)
+                end
             end
+            shapes = unique(map(typeof, shp.shapes))
+            @test GeoInterface.crs(shp) == nothing
+            @test length(shapes) == 1
+            @test length(shapes) == 1
+            @test shapes[1] == test.geomtype
+            @test eltype(shp.shapes) == Union{test.geomtype,Missing}
+            # missing and MultiPatch are not covered by the GeoInterface
+            if !(test.geomtype <: Union{Missing,Shapefile.MultiPatch})
+                @test GeoInterface.coordinates.(shp.shapes) == test.coordinates
+                # Run this a second time to test caching
+                @test GeoInterface.coordinates.(shp.shapes) == test.coordinates
+            end
+            ext = test.extent
+            @test shp.header.MBR == Shapefile.Rect(ext.X[1], ext.Y[1], ext.X[2], ext.Y[2])
+            @test GeoInterface.extent(shp) == test.extent
+            # Multipatch can't be plotted, but it's obscure anyway
+            test.geomtype == Shapefile.MultiPatch && continue
+            # Plots has no fallback recipe for a Vector{Missing}
+            length(collect(skipmissing(shp.shapes))) > 0 || continue
+            # Plots cant plot measures
+            GeoInterface.ismeasured(first(skipmissing(shp.shapes))) && continue
+            # plot(shp) # Just test that geometries actually plot
         end
-        shapes = unique(map(typeof, shp.shapes))
-        @test GeoInterface.crs(shp) == nothing
-        @test length(shapes) == 1
-        @test length(shapes) == 1
-        @test shapes[1] == test.geomtype
-        @test eltype(shp.shapes) == Union{test.geomtype,Missing}
-        # missing and MultiPatch are not covered by the GeoInterface
-        if !(test.geomtype <: Union{Missing,Shapefile.MultiPatch})
-            @test GeoInterface.coordinates.(shp.shapes) == test.coordinates
-            # Run this a second time to test caching
-            @test GeoInterface.coordinates.(shp.shapes) == test.coordinates
-        end
-        ext = test.extent
-        @test shp.header.MBR == Shapefile.Rect(ext.X[1], ext.Y[1], ext.X[2], ext.Y[2])
-        @test GeoInterface.extent(shp) == test.extent
-        # Multipatch can't be plotted, but it's obscure anyway
-        test.geomtype == Shapefile.MultiPatch && continue
-        # Plots has no fallback recipe for a Vector{Missing}
-        length(collect(skipmissing(shp.shapes))) > 0 || continue
-        # Plots cant plot measures
-        GeoInterface.ismeasured(first(skipmissing(shp.shapes))) && continue
-        # plot(shp) # Just test that geometries actually plot
     end
-end
-
-
-# Test all .shx files; the values in .shx must match the .shp offsets
-test = test_tuples[1]
-for test in test_tuples
 
-    offsets = Int32[]
-    contentlens = Int32[]
 
-    # Get the shapefile's record offsets and contentlens
-    shp = open(joinpath(@__DIR__, test.path)) do fd
-        seek(fd, 32)
-        shapeType = read(fd, Int32)
-        seek(fd, 100)
-        jltype = Shapefile.SHAPETYPE[shapeType]
+    # Test all .shx files; the values in .shx must match the .shp offsets
+    test = test_tuples[1]
+    for test in test_tuples
 
-        push!(offsets, position(fd))
-        while (!eof(fd))
+        offsets = Int32[]
+        contentlens = Int32[]
 
-            num = bswap(read(fd, Int32))
-            rlength = bswap(read(fd, Int32))
+        # Get the shapefile's record offsets and contentlens
+        shp = open(joinpath(@__DIR__, test.path)) do fd
+            seek(fd, 32)
             shapeType = read(fd, Int32)
-            if shapeType !== Int32(0)
-                read(fd, jltype)
-            end
+            seek(fd, 100)
+            jltype = Shapefile.SHAPETYPE[shapeType]
 
-            # records the offset after this geometry record
             push!(offsets, position(fd))
-        end
-    end
-    contentlens = diff(offsets)
-    offsets = offsets[1:end-1]
+            while (!eof(fd))
 
-    # Match the Index values to Shapefile offsets
-    shx =
-        open(joinpath(@__DIR__, replace(test.path, r".shp$" => ".shx"))) do fd
-            shx = read(fd, Shapefile.IndexHandle)
-            for sIdx = 1:lastindex(shx.indices)
-                @test shx.indices[sIdx].offset * 2 == offsets[sIdx]
-                @test shx.indices[sIdx].contentlen * 2 + 8 == contentlens[sIdx]
+                num = bswap(read(fd, Int32))
+                rlength = bswap(read(fd, Int32))
+                shapeType = read(fd, Int32)
+                if shapeType !== Int32(0)
+                    read(fd, jltype)
+                end
+
+                # records the offset after this geometry record
+                push!(offsets, position(fd))
             end
         end
+        contentlens = diff(offsets)
+        offsets = offsets[1:end-1]
 
-end
+        # Match the Index values to Shapefile offsets
+        shx =
+            open(joinpath(@__DIR__, replace(test.path, r".shp$" => ".shx"))) do fd
+                shx = read(fd, Shapefile.IndexHandle)
+                for sIdx = 1:lastindex(shx.indices)
+                    @test shx.indices[sIdx].offset * 2 == offsets[sIdx]
+                    @test shx.indices[sIdx].contentlen * 2 + 8 == contentlens[sIdx]
+                end
+            end
 
-end  # @testset "Loading Shapefiles"
+    end
 
-include("table.jl")
-include("writer.jl")
+    end  # @testset "Loading Shapefiles"
 
-cleanup()
+    include("table.jl")
+    include("writer.jl")
+
+    cleanup()
+
+    @testset "Aqua.jl" begin
+        Aqua.test_all(
+            Shapefile; 
+            # Exclude ambiguities from imported packages as well as GeoInterfaceRecipes,
+            # since the ambiguities there are not the kind that would actually cause problems.
+            ambiguities = (; recursive = false, exclude = [GeoInterfaceRecipes.RecipesBase.apply_recipe,]),
+            # GeoInterfaceRecipes and GeoInterfaceMakie are considered stale dependencies
+            # but are actually used in extensions on Plots and Makie respectively, so we need them!
+            stale_deps = (; ignore = [:GeoInterfaceRecipes, :GeoInterfaceMakie]), 
+            # too much headache for now - will go through this again if I'm sure 
+            # CompatHelper is working, but the tests are good flags for new versions with 
+            # suspicious behaviour.
+            deps_compat = false, 
+        )
+    end
+end

test/table.jl

@@ -52,7 +52,7 @@ ne_cities = Shapefile.Table(path(natural_earth, "ne_cities_shp"))
             if eltype(a) <: Union{Missing, String}
                 # ne_cities has non-ascii characters
                 @test all(isequal.(
-                    replace.(skipmissing(a), !isascii => x -> '_' ^ textwidth(x)),
+                    skipmissing(a),#replace.(skipmissing(a), !isascii => x -> '_' ^ textwidth(x)),
                     skipmissing(b)
                 ))
             else
@@ -105,7 +105,7 @@ wkt = "GEOGCS[\"GCS_WGS_1984\",DATUM[\"D_WGS_1984\",SPHEROID[\"WGS_1984\",637813
     )
     for r in ne_land
         @test Shapefile.shape(r) isa Shapefile.Polygon
-        @test r.featurecla === "Land"
+        @test r.featurecla == "Land"
     end
     df_land = DataFrames.DataFrame(ne_land)
     @test size(df_land) == (127, 4)