Skip to content
This repository has been archived by the owner on May 29, 2024. It is now read-only.

feat(loader): .gpkg loader #16

Merged
merged 1 commit into from
Feb 21, 2024
Merged

feat(loader): .gpkg loader #16

merged 1 commit into from
Feb 21, 2024

Conversation

jfb-h
Copy link
Contributor

@jfb-h jfb-h commented Feb 10, 2024

Reopen #14, this time with tests!

@tecosaur
Copy link
Owner

Love it, thanks for adding tests too 😍

@tecosaur
Copy link
Owner

You don't need to using ArchGDAL in runtests.jl, it will be automatically loaded as normal.

@tecosaur
Copy link
Owner

I can take care of that though if I merge this locally (might make this PR show up as "closed" though, but that's just cosmetics at the end of the day, the commit log will still show this contribution as coming from you).

@jfb-h
Copy link
Contributor Author

jfb-h commented Feb 10, 2024

You don't need to using ArchGDAL in runtests.jl, it will be automatically loaded as normal.

I thought so, but when I don't explicitly load it, the test fails with a TransformerError: Data set "eurostat-gpkg" could not be loaded in any form.. Did I miss something?

@tecosaur
Copy link
Owner

🤔 I'll have a look locally

@jfb-h
Copy link
Contributor Author

jfb-h commented Feb 20, 2024

Hi @tecosaur, I know you have a lot on your hands, but is there any chance this could be merged soonish? I have a project where I'd love to transition to DataToolkit.jl but which requires handling .gpkg files. Thanks a lot for all your effort!

@tecosaur
Copy link
Owner

Sorry for the delay (I'm beginning to get a bit busier, feel free to ping me like this though 🙂), and thanks for the contribution.

I'll give this a second look now.

@tecosaur
Copy link
Owner

I've just made a few tweaks, and realised the what the problem was/reason why ArchGDAL had to be loaded explicitly. It's because the ArchGDAL type specified in supportedtypes doesn't exist before the package is loaded, and so DataToolkitBase can't infer anything about the type. I've thought of an only slightly kludgy fix, I'll push that and merge this in a sec.

@tecosaur
Copy link
Owner

The failing CI check now seems to be

gpkg: Test Failed at /home/tec/.julia/dev/DataToolkitCommon/test/runtests.jl:28
  Expression: ArchGDAL.getlayer(geo, 0) |> length == 16
   Evaluated: 1025 == 16

let me know if this represents an actual issue, or if this should just be ... == 1025.

@jfb-h
Copy link
Contributor Author

jfb-h commented Feb 20, 2024

Ah damn, I swear I tested that locally... Yeah it should indeed be 1025, I must have mixed up the files. Thanks a lot for looking into this!

@tecosaur tecosaur merged commit 160beda into tecosaur:main Feb 21, 2024
6 of 9 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants