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

Sync metadata implementation with DataAPI.jl 1.12.0 #3189

Merged
merged 4 commits into from
Oct 3, 2022
Merged

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Sep 30, 2022

This PR should be passed through CI after JuliaData/DataAPI.jl#53 is merged and DataAPI.jl 1.12.0 is released. Other than that it should be complete (not tested, but the changes are minor - mostly simplification of the code because of dropped default implementations + adding support for default).

@bkamins bkamins requested a review from nalimilan September 30, 2022 21:04
@bkamins bkamins added the ecosystem Issues in DataFrames.jl ecosystem label Sep 30, 2022
@bkamins bkamins added this to the 1.4 milestone Sep 30, 2022
src/other/metadata.jl Outdated Show resolved Hide resolved
src/other/metadata.jl Outdated Show resolved Hide resolved
src/other/metadata.jl Outdated Show resolved Hide resolved
src/other/metadata.jl Outdated Show resolved Hide resolved
src/other/metadata.jl Outdated Show resolved Hide resolved
src/other/metadata.jl Outdated Show resolved Hide resolved
src/other/metadata.jl Outdated Show resolved Hide resolved
src/other/metadata.jl Outdated Show resolved Hide resolved
end
emptycolmetadata!(dst)
DataAPI.colmetadatasupport(typeof(src)).read || return nothing
Copy link
Member

Choose a reason for hiding this comment

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

Use an if for consistency with above?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I have changed short-circut to ifs everywhere then. After CI passes I will merge this PR.
Then I will wait one day to wait for final responses, and will make a release of DataFrames.jl 1.4 tomorrow if there are none.

test/metadata.jl Outdated Show resolved Hide resolved
@nalimilan
Copy link
Member

Nice cleanup!

@bkamins bkamins merged commit 94ac6cc into main Oct 3, 2022
@bkamins bkamins deleted the bk/update_metadata branch October 3, 2022 12:15
@bkamins
Copy link
Member Author

bkamins commented Oct 3, 2022

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecosystem Issues in DataFrames.jl ecosystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants