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

Don't export Dates.adjust #53027

Merged
merged 3 commits into from
Jan 27, 2024
Merged

Don't export Dates.adjust #53027

merged 3 commits into from
Jan 27, 2024

Conversation

jariji
Copy link
Contributor

@jariji jariji commented Jan 24, 2024

adjust(df::Dates.DateFunction, start, step, limit)
adjust(func::Function, start; step, limit)

Dates.adjust has been exported for a while, but has no docstring and is not in the manual in 1.10.

I've asked on Slack, GitHub, and Discourse if anybody is using it, and nobody piped up.

Since we're adding docstrings to everything for 1.11, it seems like a good time to remove the adjust export.

Resolves #52746.

@LilithHafner LilithHafner added needs news A NEWS entry is required for this change needs pkgeval Tests for all registered packages should be run with this change labels Jan 25, 2024
@LilithHafner
Copy link
Member

@nanosoldier runtests()

@LilithHafner
Copy link
Member

This has never been part of the public API because the public API is defined to include only things in the manual.

@mbauman
Copy link
Member

mbauman commented Jan 25, 2024

There is some usage in the ecosystem (Quantlib, QDates, OpenSSL, Miletus), but I think only QuantLib relies upon it being exported.

@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

Nanosoldier revealed two new failures:

OpenSSL extends and reexports adjust. Most of it's usage does not depend on adjust being exported. The only tests that fail are the explicit "does OpenSSL.adjust work" tests, but the tests are failfast so there could be more that fail.

Dates itself fails because it expects the symbol adjust to be public & undocumented but this unbreaks that test (this failure should come up this this PR's CI)

QuantLib did not come up in PkgEval because it is skipped. (no updates since 2020, 3 monthly downloads according to JuliaHub, 0 dependants)

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming we get Dates tests fixed as @LilithHafner mentioned

@jam-khan
Copy link
Contributor

I have added docstrings and made changes to runtests.jl for adjust as part of issue #52725 in PR #52914

@LilithHafner LilithHafner removed the needs pkgeval Tests for all registered packages should be run with this change label Jan 27, 2024
@LilithHafner LilithHafner added merge me PR is reviewed. Merge when all tests are passing dates Dates, times, and the Dates stdlib module and removed needs news A NEWS entry is required for this change labels Jan 27, 2024
@LilithHafner LilithHafner merged commit ecc14ca into JuliaLang:master Jan 27, 2024
7 of 10 checks passed
@LilithHafner LilithHafner removed the merge me PR is reviewed. Merge when all tests are passing label Jan 27, 2024
@DilumAluthge
Copy link
Member

This PR broke the doctest job in CI.

LilithHafner pushed a commit that referenced this pull request Jan 28, 2024
Reverts #53027

#53027 broke the `doctest` job in CI.
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.

Is Dates.adjust supposed to be exported?
7 participants