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

Floor / Ceil methods for Number and Period Inconsistency #34782

Closed
Nosferican opened this issue Feb 17, 2020 · 4 comments · Fixed by #37286
Closed

Floor / Ceil methods for Number and Period Inconsistency #34782

Nosferican opened this issue Feb 17, 2020 · 4 comments · Fixed by #37286
Labels
domain:dates Dates, times, and the Dates stdlib module good first issue Indicates a good issue for first-time contributors to Julia

Comments

@Nosferican
Copy link
Contributor

I just noticed that the floor / ceil methods for Number and Period have the positional arguments inverted.

floor(Int, 3.5)
using Dates
floor(DateTime("2000-01-31"), Month)

From Slack, Dates should be consistent with Base.

Related: #18574

@Nosferican Nosferican changed the title Floor / Ceil methods for Number and Period Floor / Ceil methods for Number and Period Inconsistency Feb 17, 2020
@JeffBezanson JeffBezanson added the domain:dates Dates, times, and the Dates stdlib module label Feb 18, 2020
@dkarrasch
Copy link
Member

Out of curiosity, what has changed that the arguments against this proposal from #18574 are no longer valid?

@Nosferican
Copy link
Contributor Author

I think just people keep getting confused by the order... Would it be too bad to have both methods?

ceil(dt::TimeType, p::Period) -> TimeType
ceil(p::Period, dt::TimeType) -> TimeType
ceil(x::Period, precision::T) where T <: Union{TimePeriod, Week, Day} -> T
ceil(precision::T, x::Period) where T <: Union{TimePeriod, Week, Day} -> T

@JeffBezanson JeffBezanson added the good first issue Indicates a good issue for first-time contributors to Julia label Feb 29, 2020
@rapus95
Copy link
Contributor

rapus95 commented Mar 4, 2020

As I already posted in #34810 (comment) I'm thinking that the original considerations in #18574 were right and the current behaviour is not inconsistent but missing functionality. It can be seen easily if we add a function which can round to a period while converting the time type:

round(Date, now(), Month, RoundUp) == Date(2020, 4) #at least for the month of the post 😄

Tl;Dr: rather add those missing functions and make only the 2nd argument a required one

@Moelf
Copy link
Sponsor Contributor

Moelf commented Aug 10, 2020

I'd like to work on this, if I understand this correctly, we'd like the following to pass as tests in the end?

now_ = now()
for p in (Year, Month, Day)
    for r in (RoundUp, RoundDown)
        @test round(Date, now_, p, r) == round(Date(now_), p, r)
    end
    @test floor(Date, now_, p) == round(Date, now_, p, RoundDown)
    @test ceil(Date, now_, p)  == round(Date, now_, p, RoundUp)
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dates Dates, times, and the Dates stdlib module good first issue Indicates a good issue for first-time contributors to Julia
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants