Skip to content

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

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
Member

@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