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

Also dispatch GI.isgeometry on Type. #307

Merged
merged 2 commits into from
Jun 15, 2022
Merged

Also dispatch GI.isgeometry on Type. #307

merged 2 commits into from
Jun 15, 2022

Conversation

evetion
Copy link
Collaborator

@evetion evetion commented Jun 15, 2022

This fixes a small oversight from #290.

@evetion evetion requested a review from rafaqz June 15, 2022 15:49
@rafaqz
Copy link
Collaborator

rafaqz commented Jun 15, 2022

Was wondering what was going on

rafaqz
rafaqz previously approved these changes Jun 15, 2022
@rafaqz
Copy link
Collaborator

rafaqz commented Jun 15, 2022

Well, theres still something odd going on with the tests.

@evetion
Copy link
Collaborator Author

evetion commented Jun 15, 2022

Yep, it's because GDAL v1.4 😢 @visr

@rafaqz
Copy link
Collaborator

rafaqz commented Jun 15, 2022

Pinning 1.3 works for me, that will let my other PRs get merged as well.

@evetion evetion merged commit 3a58fe6 into master Jun 15, 2022
@evetion evetion deleted the fix/geointerface branch June 15, 2022 17:17
@visr
Copy link
Collaborator

visr commented Jun 15, 2022

What was up with GDAL.jl v1.4? If it is just the regular #158 we shouldn't pin it. I tested ArchGDAL locally before making the GDAL release, I believe it was just that.

@rafaqz
Copy link
Collaborator

rafaqz commented Jun 15, 2022

I'm not sure its just that? There are a lot of nothing return values and a method error:

https://github.com/yeesian/ArchGDAL.jl/runs/6903399033?check_suite_focus=true#step:20:296

@evetion
Copy link
Collaborator Author

evetion commented Jun 15, 2022

See https://github.com/yeesian/ArchGDAL.jl/runs/6903398191?check_suite_focus=true

More tests were failing than just a tolerance it seemed, not sure why. And pinning untill we make a release that works with 1.4 seems right?

@evetion
Copy link
Collaborator Author

evetion commented Jun 15, 2022

Failures are due to OSGeo/gdal@1164ba2#diff-360c35bb4393b380a99c8d258b1a8f20dbcc9634e4ccb6e22fe7ad82593892e9L405.

So the manual flag checks implemented in #290 to fix #303 are now not needed anymore.

@visr
Copy link
Collaborator

visr commented Jun 15, 2022

Ha nice find. For all these bool integer returns I guess we need to test > 0.
At least GDAL.jl has a new testset now: JuliaGeo/GDAL.jl@4f96fac

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

Successfully merging this pull request may close these issues.

3 participants