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

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

Closed
alex-s-gardner opened this issue Oct 27, 2023 · 8 comments
Closed

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

alex-s-gardner opened this issue Oct 27, 2023 · 8 comments

Comments

@alex-s-gardner
Copy link

#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
Copy link
Member

rafaqz commented Oct 27, 2023

@evetion can we bring back that constructor?

@evetion
Copy link
Member

evetion commented Oct 28, 2023

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

@rafaqz
Copy link
Member

rafaqz commented Oct 28, 2023

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

@evetion
Copy link
Member

evetion commented Oct 28, 2023

@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
Copy link
Member

rafaqz commented Oct 28, 2023

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

@rafaqz rafaqz closed this as not planned Won't fix, can't repro, duplicate, stale Oct 28, 2023
@alex-s-gardner
Copy link
Author

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

@alex-s-gardner
Copy link
Author

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
Copy link
Member

evetion commented Oct 28, 2023

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.

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

No branches or pull requests

3 participants