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 #53692

Merged
merged 1 commit into from
Apr 20, 2024

Conversation

aplavin
Copy link
Contributor

@aplavin aplavin commented Mar 11, 2024

These methods are clearly internal: not documented, and with unclear semantics in the general case.

They lead to weird results instead of errors:

julia> using Unitful

julia> yearmonthday(1u"°")
(0.0, 12.0, 31.017453292519917)

julia> using Measurements

julia> yearmonthday(100±1000)
(1.0 ± 0.0, 4.0 ± 0.0, 10.0 ± 1000.0)

So, would be best to really remove them from public functions (hope noone relies on them).

@aplavin
Copy link
Contributor Author

aplavin commented Apr 18, 2024

bump

@quinnj quinnj merged commit 8416647 into JuliaLang:master Apr 20, 2024
8 of 11 checks passed
@giordano
Copy link
Contributor

giordano commented Apr 20, 2024

Loads of tests have been broken by this PR, it shouldn't have been merged, I'm going to revert it

@giordano
Copy link
Contributor

@aplavin this PR is now reverted. You're welcome to resubmit again a working version, but all tests and checks were failing to your changes well before your bump, and this PR shouldn't have been merged in that state. Please be more careful next time and fix the breakage before bumping.

@aplavin
Copy link
Contributor Author

aplavin commented Apr 20, 2024

Sorry, I guess I just got used to tests often being broken unrelated to the changes in PRs, and didn't look into them...

These specific changes obviously don't introduce any new functionality. I submitted them for the sole reason of getting an error message instead of a weird result back, when passing unexpected types to these Dates functions.
Didn't expect this to produce test breakages at all, I guess moving these methods is not that important and isn't worth the risk. It's definitely not the worst case of "correctness bugs" :)

@LilithHafner LilithHafner added dates Dates, times, and the Dates stdlib module minor change Marginal behavior change acceptable for a minor release labels Apr 20, 2024
@LilithHafner
Copy link
Member

LilithHafner commented Apr 20, 2024

I think these changes are appropriate, we just also need to adjust the CI tests to allow for them, and run nanosoldier to make sure people don't actually rely on them.

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 reverted This PR has since been reverted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants