Skip to content

Conversation

@asinghvi17
Copy link
Member

@asinghvi17 asinghvi17 commented Sep 21, 2024

  • Needs tests
  • needs docs

src/fallbacks.jl Outdated
Comment on lines 101 to 108
function _get_dataapi_metadata(geom::GeomType, key, default) where GeomType
if DataAPI.metadatasupport(GeomType).read
if key in DataAPI.metadatakeys(geom)
return DataAPI.metadata(geom, key; style = false)
end
end
return default
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I move this to some other location / file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe to (a new) metadata.jl? I discussed having a Geometadata.jl package with @rafaqz, but that's probably overkill for now.

@asinghvi17
Copy link
Member Author

This needs to also check the type of the returned geometrycolumns data, and if it's not a tuple of symbols (maybe it's a String, since Arrow only supports String -> String metadata), it should convert it. Does that make sense?

@rafaqz
Copy link
Member

rafaqz commented Nov 5, 2024

You mean if its a single String we should return (Symbol(str),) ?

@asinghvi17
Copy link
Member Author

Yes

@rafaqz
Copy link
Member

rafaqz commented Nov 5, 2024

Sounds pragmatic

@asinghvi17 asinghvi17 requested a review from evetion November 24, 2024 14:39
@asinghvi17
Copy link
Member Author

OK, I've added some aftercare functions here. Do we want anything else?

Copy link
Member

@rafaqz rafaqz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can we merge?

@rafaqz rafaqz merged commit 334c0ec into main Dec 13, 2024
8 of 10 checks passed
@rafaqz rafaqz deleted the as/dataapi branch December 13, 2024 22:19
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.

4 participants