-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Grab bag of Dates fixes #29509
Grab bag of Dates fixes #29509
Conversation
stdlib/Dates/src/types.jl
Outdated
@@ -350,6 +350,7 @@ function ==(a::Time, b::Time) | |||
microsecond(a) == microsecond(b) && nanosecond(a) == nanosecond(b) | |||
end | |||
(==)(x::TimeType, y::TimeType) = (===)(promote(x, y)...) | |||
Base.hash(x::Time, h::UInt) = hash(hour(x), hash(second(x), hash(second(x), h))) |
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.
Did you mean to include minute
instead of second
twice?
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.
Yeah, I was having source build problems, so I didn't get a chance to try any of this out locally. I think I've got it sorted out now.
These commits will have to be separated. The hash change is a bug fix (whether it's suitable for a point release is debatable, but it can definitely go in 1.1), but the deprecation will take longer. For now I'd recommend moving the deprecated methods to deprecated.jl as you've done, but with no warnings for now, until we decide how to handle them. |
Ok, I commented out the |
# 1.0 deprecations | ||
function (+)(x::AbstractArray{<:TimeType}, y::GeneralPeriod) | ||
# depwarn("non-broadcasted arithmetic is deprecated for Dates.TimeType; use broadcasting instead", nothing) |
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.
All the nothing
s should be updated too?
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.
to what? I admit I don't really know what should be there instead of nothing
.
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.
the name of the function that's being deprecated (in this case :(+)
)
also mark all functions that contain a call to depwarn
with @noinline
so we can print the warning with correct location info
This seems fine for now. The details of the deprecations can be settled when we decide to active them. |
Is the first commit backportable? |
I think so; this fixes the hash function to obey the correctness property, so it's definitely a bug fix. |
Ok, it seems the first commit is already included in 1.0.2: #29742. |
No description provided.