JuliaGeo / GeoFormatTypes.jl

Wrapper types for spatial data formats like well known text, KML, Proj4 strings.
https://juliageo.github.io/GeoFormatTypes.jl/stable
MIT License
6 stars 0 forks source link

PR #22 is breaking... should a major release be issued? #31

Closed alex-s-gardner closed 1 year ago

alex-s-gardner commented 1 year ago

22 is breaking.

using GeoFormatTypes v0.4.1

julia> epsg = GeoFormatTypes.EPSG(4326)
EPSG(4326)

julia> GeoFormatTypes.EPSG(epsg)
EPSG(4326)
julia> typeof(epsg) == GeoFormatTypes.EPSG
true

using GeoFormatTypes v0.4.2

julia> epsg = GeoFormatTypes.EPSG(4326)
EPSG{1}((4326,))

julia> GeoFormatTypes.EPSG(epsg)
ERROR: MethodError: no method matching EPSG(::EPSG{1})

Closest candidates are:
  EPSG(::AbstractString)
   @ GeoFormatTypes ~/.julia/packages/GeoFormatTypes/UFNTK/src/GeoFormatTypes.jl:319
  EPSG(::Tuple{Vararg{Int64, N}}) where N
   @ GeoFormatTypes ~/.julia/packages/GeoFormatTypes/UFNTK/src/GeoFormatTypes.jl:316
  EPSG(::Int64...)
   @ GeoFormatTypes ~/.julia/packages/GeoFormatTypes/UFNTK/src/GeoFormatTypes.jl:318
  ...

Stacktrace:
 [1] top-level scope
   @ REPL[6]:1
julia> typeof(epsg) == GeoFormatTypes.EPSG
false
rafaqz commented 1 year ago

@evetion can we bring back that constructor?

evetion commented 1 year ago

Yeah, I will make a PR. Although it seems like a weird API, did anyone use it as such?

rafaqz commented 1 year ago

I cant remember intentionally doing that. Did it call convert or somrthing?

evetion commented 1 year ago

@alex-s-gardner Do you actually call EPSG(::EPSG), or test for type equality? I'm sorry if we broke your code. While technically breaking (the type signature), we thought we could get away with it, as I made sure the tests stayed green. Also:

julia> a = EPSG(4326)

julia> typeof(a) <: GeoFormatTypes.EPSG
true

julia> a isa GeoFormatTypes.EPSG
true

I cant remember intentionally doing that. Did it call convert or somrthing?

Yeah, it recognized EPSG requires an Int and EPSG can convert to that directly. Doesn't seem to work out of the box for NTuples though.

rafaqz commented 1 year ago

Oh right its still actually an EPSG just not exactly the type. I misunderstood the comparison.

Thats not actually breaking, type parameter equality is not promised.

This is the reason to use <: rather than == for types

alex-s-gardner commented 1 year ago

OK, I used both of these in FastGeoProjections but if they are not considered breaking then we're all good.

alex-s-gardner commented 1 year ago

How does one assert function input to be a subtype of?

source_epsg :: EPSG

I would have thought something like source_epsg:: <: EPSG of source_epsg:: <: EPSG

but neither work and searching has proven fruitless

evetion commented 1 year ago

Exactly like you want:

function do_thing(source_epsg::EPSG)

Although I do sometimes try to write <:, but that's only for parametric types in methods.

You can derive this logically: do_thing(something) equals do_thing(something::Any) and 1 isa Any == true and typeof(1) <: Any == true.

Documentation is here.