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

default to metadata! style=:default #55

Closed
nathanrboyer opened this issue Nov 2, 2022 · 6 comments · Fixed by #56
Closed

default to metadata! style=:default #55

nathanrboyer opened this issue Nov 2, 2022 · 6 comments · Fixed by #56

Comments

@nathanrboyer
Copy link

Maybe this was discussed already, but I would think a style named :default would be defaulted to if style is not provided.
I would like to see style as an optional keyword argument so that metadata!(df, "key", "value") is allowed. (Then TableMetadataTools.jl would define a way to change what style metadata!(df, "key", "value") uses.)

@bkamins bkamins transferred this issue from JuliaData/DataFrames.jl Nov 2, 2022
@bkamins
Copy link
Member

bkamins commented Nov 2, 2022

@nathanrboyer - this is an issue for DataAPI.jl, so I am moving it.

@bkamins
Copy link
Member

bkamins commented Nov 2, 2022

@nalimilan - what do you think?

To start with I wanted setting style to be explicit, but maybe indeed allowing for :default by default is better.

@nathanrboyer - I am working on TableMetadataTools.jl so expect changes in the code.

@nathanrboyer
Copy link
Author

nathanrboyer commented Nov 2, 2022

I can write this at the top of my script to get the desired behavior, but there is probably a cleaner way with metaprogramming:
DataFrames.metadata!(df::DataFrame, key::AbstractString, value::Any) = DataFrames.metadata!(df, key, value; style=:default)

@nathanrboyer
Copy link
Author

If it was named something other than :default, then I could see forcing the user to be explicit in the main package and only providing the convenience method in the helper package.

@bkamins
Copy link
Member

bkamins commented Nov 2, 2022

I understand your rationale. Let us wait for @nalimilan to comment. The change will be easy to implement.

@nalimilan
Copy link
Member

Sounds good.

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 a pull request may close this issue.

3 participants