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

Add Quarter period #36364

Merged
merged 8 commits into from
Jul 4, 2020
Merged

Add Quarter period #36364

merged 8 commits into from
Jul 4, 2020

Conversation

matthieugomez
Copy link
Contributor

@matthieugomez matthieugomez commented Jun 19, 2020

This is the same as #35519, except that it is rebased (the only difference is to resolve the NEWS file conflict). Sorry to open a different pull request but I cannot modify the original pull request anymore — not sure what I did wrong.

@PallHaraldsson
Copy link
Contributor

I was looking at the code, and noticed:

const QUARTERDAYS = (0, 90, 181, 273)

I think this is still wrong, for a leap year:

julia> dayofquarter(Date(2020, 4, 1))
2

@matthieugomez
Copy link
Contributor Author

Good point! I think you could open an issue about it --- this is not something I have introduced with my changes.

@kshyatt kshyatt added the dates Dates, times, and the Dates stdlib module label Jun 30, 2020
@quinnj quinnj merged commit 25073d6 into JuliaLang:master Jul 4, 2020
@matthieugomez
Copy link
Contributor Author

Thanks!

@PaulSoderlind
Copy link
Contributor

Did you forget adding the following to adjusters.jl, or was it intentional?:

Base.trunc(dt::Date, p::Type{Quarter}) = firstdayofquarter(dt)
Base.trunc(dt::DateTime, p::Type{Quarter}) = DateTime(trunc(Date(dt), Quarter))

@matthieugomez
Copy link
Contributor Author

Indeed, I forgot to add these methods. Could you open a pull request to add them?

@PaulSoderlind
Copy link
Contributor

done. #38277

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dates Dates, times, and the Dates stdlib module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants