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

Grab bag of Dates fixes #29509

Merged
merged 2 commits into from
Nov 1, 2018
Merged

Grab bag of Dates fixes #29509

merged 2 commits into from
Nov 1, 2018

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Oct 4, 2018

No description provided.

@@ -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)))
Copy link
Sponsor Member

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?

Copy link
Member Author

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.

@JeffBezanson
Copy link
Sponsor Member

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.

@quinnj
Copy link
Member Author

quinnj commented Oct 4, 2018

Ok, I commented out the depwarns for now.

# 1.0 deprecations
function (+)(x::AbstractArray{<:TimeType}, y::GeneralPeriod)
# depwarn("non-broadcasted arithmetic is deprecated for Dates.TimeType; use broadcasting instead", nothing)
Copy link
Member

Choose a reason for hiding this comment

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

All the nothings should be updated too?

Copy link
Member Author

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.

Copy link
Sponsor Member

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

@JeffBezanson
Copy link
Sponsor Member

This seems fine for now. The details of the deprecations can be settled when we decide to active them.

@KristofferC
Copy link
Sponsor Member

Is the first commit backportable?

@JeffBezanson
Copy link
Sponsor Member

I think so; this fixes the hash function to obey the correctness property, so it's definitely a bug fix.

@StefanKarpinski StefanKarpinski modified the milestones: 1.1, 1.0.x Nov 18, 2018
@KristofferC
Copy link
Sponsor Member

Ok, it seems the first commit is already included in 1.0.2: #29742.

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 this pull request may close these issues.

6 participants