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

Bugfix show #177

Closed
wants to merge 6 commits into from
Closed

Bugfix show #177

wants to merge 6 commits into from

Conversation

rafaqz
Copy link
Member

@rafaqz rafaqz commented Jan 6, 2025

Fixes #176

Also fixes a few other missing variables after running JET

@evetion
Copy link
Member

evetion commented Jan 8, 2025

Tests??

@rafaqz
Copy link
Member Author

rafaqz commented Jan 8, 2025

Yeah

@asinghvi17
Copy link
Member

Looks like the new show methods don't catch everything:

julia> @show lr;
lr = GeoInterface.Wrappers.LinearRing{false, false, Vector{Tuple{Float64, Float64}}, Nothing, Nothing}([(0.4083621168668843, 0.3241081369331741), (0.6576490657994901, 0.8947885368160798), (0.28505143942419986, 0.33026739871915556), (0.6447348618937504, 0.45342604213989457), (0.21373432697077166, 0.44230337294222666), (0.10026772922087546, 0.6251846474866037), (0.9483433569917994, 0.7063875201028693), (0.707534681160356, 0.5215473041017257), (0.6162901701134593, 0.6319947001700302), (0.4083621168668843, 0.3241081369331741)], nothing, nothing)

@asinghvi17
Copy link
Member

though maybe that's two-arg show...

@rafaqz
Copy link
Member Author

rafaqz commented Jan 13, 2025

Should we also change two-arg show? I'm never clear what the rules are here

@asinghvi17
Copy link
Member

asinghvi17 commented Jan 13, 2025

Should we also change two-arg show

I think so, the issue with arrays of geometries vomiting all over the screen is still present :D

On another note, I just added this to GeometryOps, but still get this error:

https://github.com/JuliaGeo/GeometryOps.jl/actions/runs/12744697801/job/35517145517?pr=223#step:6:205

ERROR: LoadError: MethodError: no method matching show(::IOContext{IOBuffer}, ::MIME{Symbol("text/plain")}, ::GeoInterface.Wrappers.Point{false, false, Tuple{Float64, Float64}, Nothing}; show_mz::Bool, screen_ncols::Int64)
This error has been manually thrown, explicitly, so the method may exist but be intentionally marked as unimplemented.

Closest candidates are:
  show(::IO, ::MIME{Symbol("text/plain")}, ::GeoInterface.Wrappers.Point{Z, M, T, C}; show_mz) where {Z, M, T, C} got unsupported keyword argument "screen_ncols"
   @ GeoInterface ~/.julia/packages/GeoInterface/9xL2O/src/wrappers.jl:454
  show(::IO, ::MIME{Symbol("text/plain")}, ::Any) got unsupported keyword arguments "show_mz", "screen_ncols"
   @ Base multimedia.jl:47
  show(::IO, ::MIME{Symbol("text/plain")}, ::Core.TypeofBottom) got unsupported keyword arguments "show_mz", "screen_ncols"
   @ Base show.jl:567
  ...

Stacktrace:
  [1] kwerr(::@NamedTuple{show_mz::Bool, screen_ncols::Int64}, ::Function, ::IOContext{IOBuffer}, ::MIME{Symbol("text/plain")}, ::GeoInterface.Wrappers.Point{false, false, Tuple{Float64, Float64}, Nothing})
    @ Base ./error.jl:165
  [2] _nice_geom_str(geom::GeoInterface.Wrappers.Point{false, false, Tuple{Float64, Float64}, Nothing}, show_mz::Bool, compact::Bool, screen_ncols::Int64)
    @ GeoInterface.Wrappers ~/.julia/packages/GeoInterface/9xL2O/src/wrappers.jl:322

@rafaqz
Copy link
Member Author

rafaqz commented Jan 13, 2025

I think that's a separate bug ;)

(But I'll fix here too, and add 2 arg show)

src/wrappers.jl Show resolved Hide resolved
@alex-s-gardner
Copy link
Contributor

This is a pretty nasty bug, can this PR be merged?

@rafaqz
Copy link
Member Author

rafaqz commented Jan 16, 2025

It needs tests...

@asinghvi17
Copy link
Member

asinghvi17 commented Jan 20, 2025

I added some tests, would appreciate a review before merge! One issue did come up with LineString printing in compact mode (ideally it should not show the point type) but that's a stylistic choice that we can postpone till the PR is done.

this is probably the best example of a 9pm commit I've had so far...

in all seriousness we could change the name
@rafaqz rafaqz closed this Jan 20, 2025
@alex-s-gardner alex-s-gardner mentioned this pull request Jan 20, 2025
@asinghvi17
Copy link
Member

Superseded by #185

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

show bug with LibGEOS.Polygon
4 participants