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

split internal dates methods into separate functions #55597

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aplavin
Copy link
Contributor

@aplavin aplavin commented Aug 27, 2024

another attempt at #53692

@LilithHafner LilithHafner added dates Dates, times, and the Dates stdlib module minor change Marginal behavior change acceptable for a minor release re-land This relands a PR that was previously merged but was later reverted. needs pkgeval Tests for all registered packages should be run with this change labels Aug 27, 2024
@LilithHafner
Copy link
Member

I still like the changes.

Need to adjust rata2datetime and datetime2rata to use _yearmonthday, too.

@aplavin
Copy link
Contributor Author

aplavin commented Aug 27, 2024

Tests still fail, but I cannot see any specific failure in the logs.

@LilithHafner
Copy link
Member

Yeah, it's confusing to me too. I'm trying to figure out how to rerun the build in case it's a buildkite issue. I've never seen a failure like this.

@aplavin
Copy link
Contributor Author

aplavin commented Sep 9, 2024

bump... not sure what to do here

@LilithHafner
Copy link
Member

Thanks for the bump. Next steps are to fix test failures in CI and then run pkgeval. Here's a real test failure from CI:


Error During Test at /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-grannysmith-C07ZQ07RJYVY.0/build/default-grannysmith-C07ZQ07RJYVY-0/julialang/julia-master/julia-8d470900e4/share/julia/stdlib/v1.12/Dates/test/accessors.jl:59
--
  | Test threw exception
  | Expression: (y, m, d) == Dates.yearmonthday(days)
  | MethodError: no method matching yearmonthday(::Int64)
  | The function `yearmonthday` exists, but no method is defined for this combination of argument types.

@aplavin
Copy link
Contributor Author

aplavin commented Sep 10, 2024

Hmmm, some of these methods are tested directly in

@testset "yearmonthday/yearmonth/monthday" begin
# yearmonthday is the opposite of totaldays
# taking Rata Die Day # and returning proleptic Gregorian date
@test Dates.yearmonthday(-306) == (0, 2, 29)
@test Dates.yearmonth(-306) == (0, 2)
@test Dates.monthday(-306) == (2, 29)
@test Dates.yearmonthday(-305) == (0, 3, 1)
@test Dates.yearmonth(-305) == (0, 3)
@test Dates.monthday(-305) == (3, 1)
@test Dates.yearmonthday(-2) == (0, 12, 29)
@test Dates.yearmonth(-2) == (0, 12)
@test Dates.monthday(-2) == (12, 29)
@test Dates.yearmonthday(-1) == (0, 12, 30)
@test Dates.yearmonth(-1) == (0, 12)
@test Dates.monthday(-1) == (12, 30)
@test Dates.yearmonthday(0) == (0, 12, 31)
@test Dates.yearmonth(-0) == (0, 12)
@test Dates.monthday(-0) == (12, 31)
@test Dates.yearmonthday(1) == (1, 1, 1)
@test Dates.yearmonth(1) == (1, 1)
@test Dates.monthday(1) == (1, 1)
@test Dates.yearmonthday(730120) == (2000, 1, 1)
end
@testset "year/month/day" begin
# year, month, and day return the individual components
# of yearmonthday, avoiding additional calculations when possible
@test Dates.year(-1) == 0
@test Dates.month(-1) == 12
@test Dates.day(-1) == 30
@test Dates.year(0) == 0
@test Dates.month(0) == 12
@test Dates.day(0) == 31
@test Dates.year(1) == 1
@test Dates.month(1) == 1
@test Dates.day(1) == 1
@test Dates.year(730120) == 2000
@test Dates.month(730120) == 1
@test Dates.day(730120) == 1
end

Any idea whether it was intended as a public api (yearmonthday(x) and others taking any number) or not?

@LilithHafner
Copy link
Member

I don't see that usage documented, so no: not public. Those tests should be switched to testing _yearmonthday and friends. Nanosoldier can tell us if packages depend on those undocumented internals.

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@LilithHafner
Copy link
Member

The failures in Kedzi, NanoDates, and Dates look real. If this were breaking something that was ever public, I would be concerned. As is, we should investigate Kexzi and NanoDates's failures and it would be polite to make PRs that fix them but I don't view those nanosoldier failures as blocking.

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 minor change Marginal behavior change acceptable for a minor release needs pkgeval Tests for all registered packages should be run with this change re-land This relands a PR that was previously merged but was later reverted.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants