-
Notifications
You must be signed in to change notification settings - Fork 13
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
add metadata
#48
Merged
+286
−1
Merged
add metadata
#48
Changes from all commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
ff31aa7
add `getmetadata`
bkamins 96b1e39
Update Project.toml
bkamins f7cb305
Apply suggestions from code review
bkamins 77e3604
Update runtests.jl
bkamins 7d53c4b
Update src/DataAPI.jl
bkamins c28d7c5
Update test/runtests.jl
bkamins 37ef469
Update test/runtests.jl
bkamins 4617f60
Update test/runtests.jl
bkamins 5a493b6
update API
bkamins aa15118
improve test coverage
bkamins 992d1e8
change column to key
bkamins bbeda80
Apply suggestions from code review
bkamins 45b433a
Update src/DataAPI.jl
bkamins 6897052
Update src/DataAPI.jl
bkamins 2ac9c88
add colmetadata
bkamins 43cec64
fix typo
bkamins 320f9db
fix another typo
bkamins 3e7ed28
fix tests
bkamins f2e4eb1
improve contract description
bkamins 76bfba5
update error message
bkamins 9db264a
drop table-level colmetadata
bkamins b863431
Update test/runtests.jl
bkamins 0df50d7
Apply suggestions from code review
bkamins 0c685ad
add hascolmetadata for whole table
bkamins 0d75fce
new metadata style
bkamins 40edcd9
small fixes
bkamins c3d7f94
Apply suggestions from code review
bkamins a75b965
changes after code review
bkamins 47629ed
update specification
bkamins 51add3c
update docstrings
bkamins f0c9fc0
Apply suggestions from code review
bkamins be8102f
Apply suggestions from code review
bkamins 435e062
add metadata deletion
bkamins 56f98b7
fix typo
bkamins b909f01
fix another typo
bkamins 3e32920
fix tests
bkamins b2d2636
one more fix to tests
bkamins a589cee
final test fix
bkamins c612a12
improve test coverage
bkamins 15303a6
add emptymetadata! and emptycolmetadata!
bkamins 3b61e32
Update src/DataAPI.jl
bkamins edfd350
Update src/DataAPI.jl
bkamins b33b62e
make :none style more precise
bkamins c5f699e
change :none to :default
bkamins File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
name = "DataAPI" | ||
uuid = "9a962f9c-6df0-11e9-0e5d-c546b8b5ee8a" | ||
authors = ["quinnj <[email protected]>"] | ||
version = "1.10.0" | ||
version = "1.11.0" | ||
|
||
[compat] | ||
julia = "1" | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it, maybe the syntax would be more natural as
metadata!(x, key => value)
? That would allow extending this in the future to pass multiple pairs if it appears to be convenient.A counter-argument is that
setindex!
doesn't use that syntax, but it's almost never called that way sincex[key] = value
is nicer. Of course both syntaxes could be allowed as they are not ambiguous (we will probably never allow keys to be pairs).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a first reaction it makes sense.
My only reservation was that in DataFrames.jl
=>
is used for operation specification language, so we would have yet a third way to interpret=>
there.The question is how would it look for
colmetadata!
?(which does not look that nice)
Also note that
metadata!(x, key => value)
would not be allowed, you would need to writemetadata!(x, key => value; style=style)
.In summary - I would keep the things as they are here and consider what we design here a low-level API.
I assume that the extra new package planned (tentatively named TableMetadataTools.jl) will provide convenient high-level functions. In practice I even expect that if we define there:
this will cover 95% of use cases of metadata in practice.
In summary - I propose to discuss a convenience high-level API in TableMetadataTools.jl, as I expect that in that package we will drop the requirement to specify
style
which we have in low-level API, as in high level API all styles will be:note
.