-
Notifications
You must be signed in to change notification settings - Fork 32
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
equality and hash for terms and schemas #241
base: master
Are you sure you want to change the base?
Changes from all commits
0a6d78d
1ca55c0
e6f8b87
2f03a7c
ef367d7
993829b
ee16796
fcf551d
6850ecf
b62009e
68b240a
5a932c4
63e7ff4
08a6278
63860c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,19 @@ Base.merge!(a::Schema, b::Schema) = (merge!(a.schema, b.schema); a) | |
Base.keys(schema::Schema) = keys(schema.schema) | ||
Base.haskey(schema::Schema, key) = haskey(schema.schema, key) | ||
|
||
function Base.:(==)(first::Schema, second::Schema) | ||
first === second && return true | ||
first.schema === second.schema && return true | ||
length(first.schema) != length(second.schema) && return false | ||
for key in keys(first) | ||
!haskey(second, key) && return false | ||
second[key] != first[key] && return false | ||
end | ||
true | ||
Comment on lines
+59
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this identical to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm I think you're right. Probably fine to just check |
||
end | ||
|
||
Base.hash(schema::Schema, h::UInt) = hash(schema.schema, h) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sometimes (e.g. for arrays and tuples) we add an arbitrary constant to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I wondered about that too. Easy to do here, a bit tricker in the case of the terms (where the types might not be the asme for things that we want to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And that can "corrupt" the type of containers because of the ahem zealous use of type parameters in this codebase There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't necessarily need to hash the type: you can just define a constant and use it for all types which compare |
||
|
||
""" | ||
schema([terms::AbstractVector{<:AbstractTerm}, ]data, hints::Dict{Symbol}) | ||
schema(term::AbstractTerm, data, hints::Dict{Symbol}) | ||
|
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.
Note that this will be wrong if the dict contains
missing
(recursively). Can this happen?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 key or a value? Not possible either way at the moment (unless it's a pathological manuallly constructed instance)
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 value I think, as for keys dicts use
isequal
. Note that this also applies if the value contains an object which contains a missing value (whatever the number of recursions).